| Message ID | 20240223052617.1867132-1-schalla@marvell.com (mailing list archive) |
|---|---|
| State | Changes Requested |
| Headers | show |
| Series | [v3] virtio-pci: correctly set virtio pci queue mem multiplier | expand |
Hi Srujana, On 23/2/24 06:26, Srujana Challa wrote: > Currently, virtio_pci_queue_mem_mult function always returns 4K > when VIRTIO_PCI_FLAG_PAGE_PER_VQ is set. But this won't > work for vhost vdpa when host has page size other than 4K. > This patch introduces a new property(host-page-per-vq) for vdpa > use case to fix the same. > > Signed-off-by: Srujana Challa <schalla@marvell.com> > --- > v2->v3: > - Modified property name, page-per-vdpa-vq to host-page-per-vq. > > v1->v2: > - Introduced a new property to get virtqueue mem multiplier for > vdpa use case. > > hw/virtio/virtio-pci.c | 10 ++++++++-- > include/hw/virtio/virtio-pci.h | 5 +++++ > 2 files changed, 13 insertions(+), 2 deletions(-) > > diff --git a/hw/virtio/virtio-pci.c b/hw/virtio/virtio-pci.c > index 1a7039fb0c..f29e60830b 100644 > --- a/hw/virtio/virtio-pci.c > +++ b/hw/virtio/virtio-pci.c > @@ -320,8 +320,12 @@ static bool virtio_pci_ioeventfd_enabled(DeviceState *d) > > static inline int virtio_pci_queue_mem_mult(struct VirtIOPCIProxy *proxy) > { > - return (proxy->flags & VIRTIO_PCI_FLAG_PAGE_PER_VQ) ? > - QEMU_VIRTIO_PCI_QUEUE_MEM_MULT : 4; > + if (proxy->flags & VIRTIO_PCI_FLAG_PAGE_PER_VQ) > + return QEMU_VIRTIO_PCI_QUEUE_MEM_MULT; > + else if (proxy->flags & VIRTIO_PCI_FLAG_HOST_PAGE_PER_VQ) > + return qemu_real_host_page_size(); > + else > + return 4; > } Per our coding style, this code should use braces: https://www.qemu.org/docs/master/devel/style.html#block-structure Regards, Phil.
On Fri, Feb 23, 2024 at 10:56:17AM +0530, Srujana Challa wrote: > Currently, virtio_pci_queue_mem_mult function always returns 4K > when VIRTIO_PCI_FLAG_PAGE_PER_VQ is set. But this won't > work for vhost vdpa when host has page size other than 4K. > This patch introduces a new property(host-page-per-vq) for vdpa > use case to fix the same. > > Signed-off-by: Srujana Challa <schalla@marvell.com> Looks good. I'd like to fail realize if both (proxy->flags & VIRTIO_PCI_FLAG_PAGE_PER_VQ) and (proxy->flags & VIRTIO_PCI_FLAG_HOST_PAGE_PER_VQ) so users do not start depending on this combination. > --- > v2->v3: > - Modified property name, page-per-vdpa-vq to host-page-per-vq. > > v1->v2: > - Introduced a new property to get virtqueue mem multiplier for > vdpa use case. > > hw/virtio/virtio-pci.c | 10 ++++++++-- > include/hw/virtio/virtio-pci.h | 5 +++++ > 2 files changed, 13 insertions(+), 2 deletions(-) > > diff --git a/hw/virtio/virtio-pci.c b/hw/virtio/virtio-pci.c > index 1a7039fb0c..f29e60830b 100644 > --- a/hw/virtio/virtio-pci.c > +++ b/hw/virtio/virtio-pci.c > @@ -320,8 +320,12 @@ static bool virtio_pci_ioeventfd_enabled(DeviceState *d) > > static inline int virtio_pci_queue_mem_mult(struct VirtIOPCIProxy *proxy) > { > - return (proxy->flags & VIRTIO_PCI_FLAG_PAGE_PER_VQ) ? > - QEMU_VIRTIO_PCI_QUEUE_MEM_MULT : 4; > + if (proxy->flags & VIRTIO_PCI_FLAG_PAGE_PER_VQ) > + return QEMU_VIRTIO_PCI_QUEUE_MEM_MULT; > + else if (proxy->flags & VIRTIO_PCI_FLAG_HOST_PAGE_PER_VQ) > + return qemu_real_host_page_size(); > + else > + return 4; > } > > static int virtio_pci_ioeventfd_assign(DeviceState *d, EventNotifier *notifier, > @@ -2301,6 +2305,8 @@ static Property virtio_pci_properties[] = { > VIRTIO_PCI_FLAG_INIT_FLR_BIT, true), > DEFINE_PROP_BIT("aer", VirtIOPCIProxy, flags, > VIRTIO_PCI_FLAG_AER_BIT, false), > + DEFINE_PROP_BIT("host-page-per-vq", VirtIOPCIProxy, flags, > + VIRTIO_PCI_FLAG_HOST_PAGE_PER_VQ_BIT, false), > DEFINE_PROP_END_OF_LIST(), > }; > > diff --git a/include/hw/virtio/virtio-pci.h b/include/hw/virtio/virtio-pci.h > index 59d88018c1..81b6de4291 100644 > --- a/include/hw/virtio/virtio-pci.h > +++ b/include/hw/virtio/virtio-pci.h > @@ -43,6 +43,7 @@ enum { > VIRTIO_PCI_FLAG_INIT_FLR_BIT, > VIRTIO_PCI_FLAG_AER_BIT, > VIRTIO_PCI_FLAG_ATS_PAGE_ALIGNED_BIT, > + VIRTIO_PCI_FLAG_HOST_PAGE_PER_VQ_BIT, > }; > > /* Need to activate work-arounds for buggy guests at vmstate load. */ > @@ -89,6 +90,10 @@ enum { > #define VIRTIO_PCI_FLAG_ATS_PAGE_ALIGNED \ > (1 << VIRTIO_PCI_FLAG_ATS_PAGE_ALIGNED_BIT) > > +/* page per vdpa vq flag to be used for vhost vdpa backends */ > +#define VIRTIO_PCI_FLAG_HOST_PAGE_PER_VQ \ > + (1 << VIRTIO_PCI_FLAG_HOST_PAGE_PER_VQ_BIT) > + > typedef struct { > MSIMessage msg; > int virq; > -- > 2.25.1
> Subject: [EXT] Re: [PATCH v3] virtio-pci: correctly set virtio pci queue mem > multiplier > > External Email > > ---------------------------------------------------------------------- > On Fri, Feb 23, 2024 at 10:56:17AM +0530, Srujana Challa wrote: > > Currently, virtio_pci_queue_mem_mult function always returns 4K when > > VIRTIO_PCI_FLAG_PAGE_PER_VQ is set. But this won't work for vhost vdpa > > when host has page size other than 4K. > > This patch introduces a new property(host-page-per-vq) for vdpa use > > case to fix the same. > > > > Signed-off-by: Srujana Challa <schalla@marvell.com> > > Looks good. I'd like to fail realize if both > (proxy->flags & VIRTIO_PCI_FLAG_PAGE_PER_VQ) and > (proxy->flags & VIRTIO_PCI_FLAG_HOST_PAGE_PER_VQ) so users do not > start depending on this combination. Could you confirm if we can add assertion for this case in virtio_pci_mem_mult() function? > > > > > --- > > v2->v3: > > - Modified property name, page-per-vdpa-vq to host-page-per-vq. > > > > v1->v2: > > - Introduced a new property to get virtqueue mem multiplier for > > vdpa use case. > > > > hw/virtio/virtio-pci.c | 10 ++++++++-- > > include/hw/virtio/virtio-pci.h | 5 +++++ > > 2 files changed, 13 insertions(+), 2 deletions(-) > > > > diff --git a/hw/virtio/virtio-pci.c b/hw/virtio/virtio-pci.c index > > 1a7039fb0c..f29e60830b 100644 > > --- a/hw/virtio/virtio-pci.c > > +++ b/hw/virtio/virtio-pci.c > > @@ -320,8 +320,12 @@ static bool > > virtio_pci_ioeventfd_enabled(DeviceState *d) > > > > static inline int virtio_pci_queue_mem_mult(struct VirtIOPCIProxy > > *proxy) { > > - return (proxy->flags & VIRTIO_PCI_FLAG_PAGE_PER_VQ) ? > > - QEMU_VIRTIO_PCI_QUEUE_MEM_MULT : 4; > > + if (proxy->flags & VIRTIO_PCI_FLAG_PAGE_PER_VQ) > > + return QEMU_VIRTIO_PCI_QUEUE_MEM_MULT; > > + else if (proxy->flags & VIRTIO_PCI_FLAG_HOST_PAGE_PER_VQ) > > + return qemu_real_host_page_size(); > > + else > > + return 4; > > } > > > > static int virtio_pci_ioeventfd_assign(DeviceState *d, EventNotifier > > *notifier, @@ -2301,6 +2305,8 @@ static Property virtio_pci_properties[] = > { > > VIRTIO_PCI_FLAG_INIT_FLR_BIT, true), > > DEFINE_PROP_BIT("aer", VirtIOPCIProxy, flags, > > VIRTIO_PCI_FLAG_AER_BIT, false), > > + DEFINE_PROP_BIT("host-page-per-vq", VirtIOPCIProxy, flags, > > + VIRTIO_PCI_FLAG_HOST_PAGE_PER_VQ_BIT, false), > > DEFINE_PROP_END_OF_LIST(), > > }; > > > > diff --git a/include/hw/virtio/virtio-pci.h > > b/include/hw/virtio/virtio-pci.h index 59d88018c1..81b6de4291 100644 > > --- a/include/hw/virtio/virtio-pci.h > > +++ b/include/hw/virtio/virtio-pci.h > > @@ -43,6 +43,7 @@ enum { > > VIRTIO_PCI_FLAG_INIT_FLR_BIT, > > VIRTIO_PCI_FLAG_AER_BIT, > > VIRTIO_PCI_FLAG_ATS_PAGE_ALIGNED_BIT, > > + VIRTIO_PCI_FLAG_HOST_PAGE_PER_VQ_BIT, > > }; > > > > /* Need to activate work-arounds for buggy guests at vmstate load. */ > > @@ -89,6 +90,10 @@ enum { #define > VIRTIO_PCI_FLAG_ATS_PAGE_ALIGNED \ > > (1 << VIRTIO_PCI_FLAG_ATS_PAGE_ALIGNED_BIT) > > > > +/* page per vdpa vq flag to be used for vhost vdpa backends */ > > +#define VIRTIO_PCI_FLAG_HOST_PAGE_PER_VQ \ > > + (1 << VIRTIO_PCI_FLAG_HOST_PAGE_PER_VQ_BIT) > > + > > typedef struct { > > MSIMessage msg; > > int virq; > > -- > > 2.25.1
On Wed, Feb 28, 2024 at 06:13:03AM +0000, Srujana Challa wrote: > > Subject: [EXT] Re: [PATCH v3] virtio-pci: correctly set virtio pci queue mem > > multiplier > > > > External Email > > > > ---------------------------------------------------------------------- > > On Fri, Feb 23, 2024 at 10:56:17AM +0530, Srujana Challa wrote: > > > Currently, virtio_pci_queue_mem_mult function always returns 4K when > > > VIRTIO_PCI_FLAG_PAGE_PER_VQ is set. But this won't work for vhost vdpa > > > when host has page size other than 4K. > > > This patch introduces a new property(host-page-per-vq) for vdpa use > > > case to fix the same. > > > > > > Signed-off-by: Srujana Challa <schalla@marvell.com> > > > > Looks good. I'd like to fail realize if both > > (proxy->flags & VIRTIO_PCI_FLAG_PAGE_PER_VQ) and > > (proxy->flags & VIRTIO_PCI_FLAG_HOST_PAGE_PER_VQ) so users do not > > start depending on this combination. > Could you confirm if we can add assertion for this case in virtio_pci_mem_mult() function? No, reporting an error would be better since it's user-triggerable - it is not nice to report user errors through assertions, assertions are for conditions that can not be reached. > > > > > > > > > --- > > > v2->v3: > > > - Modified property name, page-per-vdpa-vq to host-page-per-vq. > > > > > > v1->v2: > > > - Introduced a new property to get virtqueue mem multiplier for > > > vdpa use case. > > > > > > hw/virtio/virtio-pci.c | 10 ++++++++-- > > > include/hw/virtio/virtio-pci.h | 5 +++++ > > > 2 files changed, 13 insertions(+), 2 deletions(-) > > > > > > diff --git a/hw/virtio/virtio-pci.c b/hw/virtio/virtio-pci.c index > > > 1a7039fb0c..f29e60830b 100644 > > > --- a/hw/virtio/virtio-pci.c > > > +++ b/hw/virtio/virtio-pci.c > > > @@ -320,8 +320,12 @@ static bool > > > virtio_pci_ioeventfd_enabled(DeviceState *d) > > > > > > static inline int virtio_pci_queue_mem_mult(struct VirtIOPCIProxy > > > *proxy) { > > > - return (proxy->flags & VIRTIO_PCI_FLAG_PAGE_PER_VQ) ? > > > - QEMU_VIRTIO_PCI_QUEUE_MEM_MULT : 4; > > > + if (proxy->flags & VIRTIO_PCI_FLAG_PAGE_PER_VQ) > > > + return QEMU_VIRTIO_PCI_QUEUE_MEM_MULT; > > > + else if (proxy->flags & VIRTIO_PCI_FLAG_HOST_PAGE_PER_VQ) > > > + return qemu_real_host_page_size(); > > > + else > > > + return 4; > > > } > > > > > > static int virtio_pci_ioeventfd_assign(DeviceState *d, EventNotifier > > > *notifier, @@ -2301,6 +2305,8 @@ static Property virtio_pci_properties[] = > > { > > > VIRTIO_PCI_FLAG_INIT_FLR_BIT, true), > > > DEFINE_PROP_BIT("aer", VirtIOPCIProxy, flags, > > > VIRTIO_PCI_FLAG_AER_BIT, false), > > > + DEFINE_PROP_BIT("host-page-per-vq", VirtIOPCIProxy, flags, > > > + VIRTIO_PCI_FLAG_HOST_PAGE_PER_VQ_BIT, false), > > > DEFINE_PROP_END_OF_LIST(), > > > }; > > > > > > diff --git a/include/hw/virtio/virtio-pci.h > > > b/include/hw/virtio/virtio-pci.h index 59d88018c1..81b6de4291 100644 > > > --- a/include/hw/virtio/virtio-pci.h > > > +++ b/include/hw/virtio/virtio-pci.h > > > @@ -43,6 +43,7 @@ enum { > > > VIRTIO_PCI_FLAG_INIT_FLR_BIT, > > > VIRTIO_PCI_FLAG_AER_BIT, > > > VIRTIO_PCI_FLAG_ATS_PAGE_ALIGNED_BIT, > > > + VIRTIO_PCI_FLAG_HOST_PAGE_PER_VQ_BIT, > > > }; > > > > > > /* Need to activate work-arounds for buggy guests at vmstate load. */ > > > @@ -89,6 +90,10 @@ enum { #define > > VIRTIO_PCI_FLAG_ATS_PAGE_ALIGNED \ > > > (1 << VIRTIO_PCI_FLAG_ATS_PAGE_ALIGNED_BIT) > > > > > > +/* page per vdpa vq flag to be used for vhost vdpa backends */ > > > +#define VIRTIO_PCI_FLAG_HOST_PAGE_PER_VQ \ > > > + (1 << VIRTIO_PCI_FLAG_HOST_PAGE_PER_VQ_BIT) > > > + > > > typedef struct { > > > MSIMessage msg; > > > int virq; > > > -- > > > 2.25.1
> Subject: Re: [EXT] Re: [PATCH v3] virtio-pci: correctly set virtio pci queue mem > multiplier > > On Wed, Feb 28, 2024 at 06:13:03AM +0000, Srujana Challa wrote: > > > Subject: [EXT] Re: [PATCH v3] virtio-pci: correctly set virtio pci > > > queue mem multiplier > > > > > > External Email > > > > > > -------------------------------------------------------------------- > > > -- On Fri, Feb 23, 2024 at 10:56:17AM +0530, Srujana Challa wrote: > > > > Currently, virtio_pci_queue_mem_mult function always returns 4K > > > > when VIRTIO_PCI_FLAG_PAGE_PER_VQ is set. But this won't work for > > > > vhost vdpa when host has page size other than 4K. > > > > This patch introduces a new property(host-page-per-vq) for vdpa > > > > use case to fix the same. > > > > > > > > Signed-off-by: Srujana Challa <schalla@marvell.com> > > > > > > Looks good. I'd like to fail realize if both > > > (proxy->flags & VIRTIO_PCI_FLAG_PAGE_PER_VQ) and > > > (proxy->flags & VIRTIO_PCI_FLAG_HOST_PAGE_PER_VQ) so users do not > > > start depending on this combination. > > Could you confirm if we can add assertion for this case in > virtio_pci_mem_mult() function? > > No, reporting an error would be better since it's user-triggerable - it is not nice > to report user errors through assertions, assertions are for conditions that can > not be reached. Yes, but returning error from virtio_pci_queue_mem_mult() would need changes In multiple places right? > > > > > > > > > > > > > > > --- > > > > v2->v3: > > > > - Modified property name, page-per-vdpa-vq to host-page-per-vq. > > > > > > > > v1->v2: > > > > - Introduced a new property to get virtqueue mem multiplier for > > > > vdpa use case. > > > > > > > > hw/virtio/virtio-pci.c | 10 ++++++++-- > > > > include/hw/virtio/virtio-pci.h | 5 +++++ > > > > 2 files changed, 13 insertions(+), 2 deletions(-) > > > > > > > > diff --git a/hw/virtio/virtio-pci.c b/hw/virtio/virtio-pci.c index > > > > 1a7039fb0c..f29e60830b 100644 > > > > --- a/hw/virtio/virtio-pci.c > > > > +++ b/hw/virtio/virtio-pci.c > > > > @@ -320,8 +320,12 @@ static bool > > > > virtio_pci_ioeventfd_enabled(DeviceState *d) > > > > > > > > static inline int virtio_pci_queue_mem_mult(struct VirtIOPCIProxy > > > > *proxy) { > > > > - return (proxy->flags & VIRTIO_PCI_FLAG_PAGE_PER_VQ) ? > > > > - QEMU_VIRTIO_PCI_QUEUE_MEM_MULT : 4; > > > > + if (proxy->flags & VIRTIO_PCI_FLAG_PAGE_PER_VQ) > > > > + return QEMU_VIRTIO_PCI_QUEUE_MEM_MULT; > > > > + else if (proxy->flags & VIRTIO_PCI_FLAG_HOST_PAGE_PER_VQ) > > > > + return qemu_real_host_page_size(); > > > > + else > > > > + return 4; > > > > } > > > > > > > > static int virtio_pci_ioeventfd_assign(DeviceState *d, > > > > EventNotifier *notifier, @@ -2301,6 +2305,8 @@ static Property > > > > virtio_pci_properties[] = > > > { > > > > VIRTIO_PCI_FLAG_INIT_FLR_BIT, true), > > > > DEFINE_PROP_BIT("aer", VirtIOPCIProxy, flags, > > > > VIRTIO_PCI_FLAG_AER_BIT, false), > > > > + DEFINE_PROP_BIT("host-page-per-vq", VirtIOPCIProxy, flags, > > > > + VIRTIO_PCI_FLAG_HOST_PAGE_PER_VQ_BIT, false), > > > > DEFINE_PROP_END_OF_LIST(), > > > > }; > > > > > > > > diff --git a/include/hw/virtio/virtio-pci.h > > > > b/include/hw/virtio/virtio-pci.h index 59d88018c1..81b6de4291 > > > > 100644 > > > > --- a/include/hw/virtio/virtio-pci.h > > > > +++ b/include/hw/virtio/virtio-pci.h > > > > @@ -43,6 +43,7 @@ enum { > > > > VIRTIO_PCI_FLAG_INIT_FLR_BIT, > > > > VIRTIO_PCI_FLAG_AER_BIT, > > > > VIRTIO_PCI_FLAG_ATS_PAGE_ALIGNED_BIT, > > > > + VIRTIO_PCI_FLAG_HOST_PAGE_PER_VQ_BIT, > > > > }; > > > > > > > > /* Need to activate work-arounds for buggy guests at vmstate > > > > load. */ @@ -89,6 +90,10 @@ enum { #define > > > VIRTIO_PCI_FLAG_ATS_PAGE_ALIGNED \ > > > > (1 << VIRTIO_PCI_FLAG_ATS_PAGE_ALIGNED_BIT) > > > > > > > > +/* page per vdpa vq flag to be used for vhost vdpa backends */ > > > > +#define VIRTIO_PCI_FLAG_HOST_PAGE_PER_VQ \ > > > > + (1 << VIRTIO_PCI_FLAG_HOST_PAGE_PER_VQ_BIT) > > > > + > > > > typedef struct { > > > > MSIMessage msg; > > > > int virq; > > > > -- > > > > 2.25.1
diff --git a/hw/virtio/virtio-pci.c b/hw/virtio/virtio-pci.c index 1a7039fb0c..f29e60830b 100644 --- a/hw/virtio/virtio-pci.c +++ b/hw/virtio/virtio-pci.c @@ -320,8 +320,12 @@ static bool virtio_pci_ioeventfd_enabled(DeviceState *d) static inline int virtio_pci_queue_mem_mult(struct VirtIOPCIProxy *proxy) { - return (proxy->flags & VIRTIO_PCI_FLAG_PAGE_PER_VQ) ? - QEMU_VIRTIO_PCI_QUEUE_MEM_MULT : 4; + if (proxy->flags & VIRTIO_PCI_FLAG_PAGE_PER_VQ) + return QEMU_VIRTIO_PCI_QUEUE_MEM_MULT; + else if (proxy->flags & VIRTIO_PCI_FLAG_HOST_PAGE_PER_VQ) + return qemu_real_host_page_size(); + else + return 4; } static int virtio_pci_ioeventfd_assign(DeviceState *d, EventNotifier *notifier, @@ -2301,6 +2305,8 @@ static Property virtio_pci_properties[] = { VIRTIO_PCI_FLAG_INIT_FLR_BIT, true), DEFINE_PROP_BIT("aer", VirtIOPCIProxy, flags, VIRTIO_PCI_FLAG_AER_BIT, false), + DEFINE_PROP_BIT("host-page-per-vq", VirtIOPCIProxy, flags, + VIRTIO_PCI_FLAG_HOST_PAGE_PER_VQ_BIT, false), DEFINE_PROP_END_OF_LIST(), }; diff --git a/include/hw/virtio/virtio-pci.h b/include/hw/virtio/virtio-pci.h index 59d88018c1..81b6de4291 100644 --- a/include/hw/virtio/virtio-pci.h +++ b/include/hw/virtio/virtio-pci.h @@ -43,6 +43,7 @@ enum { VIRTIO_PCI_FLAG_INIT_FLR_BIT, VIRTIO_PCI_FLAG_AER_BIT, VIRTIO_PCI_FLAG_ATS_PAGE_ALIGNED_BIT, + VIRTIO_PCI_FLAG_HOST_PAGE_PER_VQ_BIT, }; /* Need to activate work-arounds for buggy guests at vmstate load. */ @@ -89,6 +90,10 @@ enum { #define VIRTIO_PCI_FLAG_ATS_PAGE_ALIGNED \ (1 << VIRTIO_PCI_FLAG_ATS_PAGE_ALIGNED_BIT) +/* page per vdpa vq flag to be used for vhost vdpa backends */ +#define VIRTIO_PCI_FLAG_HOST_PAGE_PER_VQ \ + (1 << VIRTIO_PCI_FLAG_HOST_PAGE_PER_VQ_BIT) + typedef struct { MSIMessage msg; int virq;
Currently, virtio_pci_queue_mem_mult function always returns 4K when VIRTIO_PCI_FLAG_PAGE_PER_VQ is set. But this won't work for vhost vdpa when host has page size other than 4K. This patch introduces a new property(host-page-per-vq) for vdpa use case to fix the same. Signed-off-by: Srujana Challa <schalla@marvell.com> --- v2->v3: - Modified property name, page-per-vdpa-vq to host-page-per-vq. v1->v2: - Introduced a new property to get virtqueue mem multiplier for vdpa use case. hw/virtio/virtio-pci.c | 10 ++++++++-- include/hw/virtio/virtio-pci.h | 5 +++++ 2 files changed, 13 insertions(+), 2 deletions(-)