[PATCH] Add page_per_vq flag to the 'driver' element of virtio devices

[Gavi Teitz]

On 4/29/21 5:43 PM, Jonathon Jongsma wrote:
> On Thu, 2021-04-29 at 14:12 +0300, Gavi Teitz wrote:
>> https://nam11.safelinks.protection.outlook.com/?url=https%3A%2F%2Fbugzilla.redhat.com%2Fshow_bug.cgi%3Fid%3D1925363&data=04%7C01%7Cgavi%40nvidia.com%7C3a0a9dc902bd4ddb90fa08d90b1d3ac9%7C43083d15727340c1b7db39efd9ccc17a%7C0%7C0%7C637553042452946162%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C1000&sdata=0xd9ZcmXQJPT%2FeqT%2Fs%2B0KOc5HnMN447pE6r7V6bc%2B98%3D&reserved=0
>>
>> Add support for setting the page-per-vq flag, which is important for
>> vdpa with vhost-user performance.
>>
>> Signed-off-by: Gavi Teitz <gavi at nvidia.com>
>>
> [snip]
>
>> diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c
>> index 9d98f48..0350fde 100644
>> --- a/src/conf/domain_conf.c
>> +++ b/src/conf/domain_conf.c
>> @@ -10446,6 +10446,7 @@ virDomainNetDefParseXML(virDomainXMLOption
>> *xmlopt,
>>       g_autofree char *queues = NULL;
>>       g_autofree char *rx_queue_size = NULL;
>>       g_autofree char *tx_queue_size = NULL;
>> +    g_autofree char *page_per_vq = NULL;
>>       g_autofree char *filter = NULL;
>>       g_autofree char *internal = NULL;
>>       g_autofree char *mode = NULL;
>> @@ -10615,6 +10616,7 @@ virDomainNetDefParseXML(virDomainXMLOption
>> *xmlopt,
>>                   queues = virXMLPropString(cur, "queues");
>>                   rx_queue_size = virXMLPropString(cur,
>> "rx_queue_size");
>>                   tx_queue_size = virXMLPropString(cur,
>> "tx_queue_size");
>> +                page_per_vq = virXMLPropString(cur, "page_per_vq");
>>   
>>                   if (virDomainVirtioOptionsParseXML(cur, &def-
>>> virtio) < 0)
>>                       goto error;
>> @@ -11041,6 +11043,15 @@ virDomainNetDefParseXML(virDomainXMLOption
>> *xmlopt,
>>               }
>>               def->driver.virtio.tx_queue_size = q;
>>           }
>> +        if (page_per_vq) {
>> +            if ((val = virTristateSwitchTypeFromString(page_per_vq))
>> <= 0) {
>> +                virReportError(VIR_ERR_CONFIG_UNSUPPORTED,
>> +                               _("unknown page_per_vq mode '%s'"),
>> +                               page_per_vq);
>> +                goto error;
>> +            }
>> +            def->driver.virtio.page_per_vq = val;
>> +        }
> Tim Wiederhake has been doing a bunch of refactoring work in this area
> changing code like this to use the newish virXMLProp* functions.
>
> So I think this whole chunk could instead be something like:
>
> if (virXMLPropTristateSwitch(cur, "page_per_vq", VIR_XML_PROP_NONE,
> 			     &def->driver.virtio.page_per_vq) < 0)
>      return -1;
>
>
> (untested)
>
> Jonathon
>
Assigning def->driver.virtio.page_per_vq in the initial parsing loop 
with the suggested lines would prevent the assignment from being 
conditioned upon virDomainNetIsVirtioModel() as it currently is.

Is it not necessary to enforce this condition, or should I instead do 
the assignment as I currently do, after the parsing loop and under the 
condition, and use virXPathNode("./driver", ctxt) to get the node?