Re: [PATCH v4 1/5] qdev: Introduce DEFINE_PROP_RESERVED_REGION
Hi Markus, On 6/24/20 10:10 AM, Markus Armbruster wrote: > Auger Eric writes: > >> Hi Markus, >> >> On 6/23/20 5:13 PM, Markus Armbruster wrote: >>> Eric Auger writes: >>> Introduce a new property defining a reserved region: ::. This will be used to encode reserved IOVA regions. For instance, in virtio-iommu use case, reserved IOVA regions will be passed by the machine code to the virtio-iommu-pci device (an array of those). The type of the reserved region will match the virtio_iommu_probe_resv_mem subtype value: - VIRTIO_IOMMU_RESV_MEM_T_RESERVED (0) - VIRTIO_IOMMU_RESV_MEM_T_MSI (1) on PC/Q35 machine, this will be used to inform the virtio-iommu-pci device it should bypass the MSI region. The reserved region will be: 0xfee0:0xfeef:1. On ARM, we can declare the ITS MSI doorbell as an MSI region to prevent MSIs from being mapped on guest side. Signed-off-by: Eric Auger Reviewed-by: Markus Armbruster --- v3 -> v4: - use ':' instead of commas as separators. - rearrange error messages - check snprintf returned value - dared to keep Markus' R-b despite those changes --- include/exec/memory.h| 6 +++ include/hw/qdev-properties.h | 3 ++ include/qemu/typedefs.h | 1 + hw/core/qdev-properties.c| 89 4 files changed, 99 insertions(+) diff --git a/include/exec/memory.h b/include/exec/memory.h index 7207025bd4..d7a53b96cc 100644 --- a/include/exec/memory.h +++ b/include/exec/memory.h @@ -51,6 +51,12 @@ extern bool global_dirty_log; typedef struct MemoryRegionOps MemoryRegionOps; +struct ReservedRegion { +hwaddr low; +hwaddr high; +unsigned int type; >>> >>> Suggest to s/unsigned int/unsigned/. >>> +}; + typedef struct IOMMUTLBEntry IOMMUTLBEntry; /* See address_space_translate: bit 0 is read, bit 1 is write. */ diff --git a/include/hw/qdev-properties.h b/include/hw/qdev-properties.h index 5252bb6b1a..95d0e7201d 100644 --- a/include/hw/qdev-properties.h +++ b/include/hw/qdev-properties.h @@ -19,6 +19,7 @@ extern const PropertyInfo qdev_prop_string; extern const PropertyInfo qdev_prop_chr; extern const PropertyInfo qdev_prop_tpm; extern const PropertyInfo qdev_prop_macaddr; +extern const PropertyInfo qdev_prop_reserved_region; extern const PropertyInfo qdev_prop_on_off_auto; extern const PropertyInfo qdev_prop_multifd_compression; extern const PropertyInfo qdev_prop_losttickpolicy; @@ -184,6 +185,8 @@ extern const PropertyInfo qdev_prop_pcie_link_width; DEFINE_PROP(_n, _s, _f, qdev_prop_drive_iothread, BlockBackend *) #define DEFINE_PROP_MACADDR(_n, _s, _f) \ DEFINE_PROP(_n, _s, _f, qdev_prop_macaddr, MACAddr) +#define DEFINE_PROP_RESERVED_REGION(_n, _s, _f) \ +DEFINE_PROP(_n, _s, _f, qdev_prop_reserved_region, ReservedRegion) #define DEFINE_PROP_ON_OFF_AUTO(_n, _s, _f, _d) \ DEFINE_PROP_SIGNED(_n, _s, _f, _d, qdev_prop_on_off_auto, OnOffAuto) #define DEFINE_PROP_MULTIFD_COMPRESSION(_n, _s, _f, _d) \ diff --git a/include/qemu/typedefs.h b/include/qemu/typedefs.h index ce4a78b687..15f5047bf1 100644 --- a/include/qemu/typedefs.h +++ b/include/qemu/typedefs.h @@ -58,6 +58,7 @@ typedef struct ISABus ISABus; typedef struct ISADevice ISADevice; typedef struct IsaDma IsaDma; typedef struct MACAddr MACAddr; +typedef struct ReservedRegion ReservedRegion; typedef struct MachineClass MachineClass; typedef struct MachineState MachineState; typedef struct MemoryListener MemoryListener; diff --git a/hw/core/qdev-properties.c b/hw/core/qdev-properties.c index ead35d7ffd..193d0d95f9 100644 --- a/hw/core/qdev-properties.c +++ b/hw/core/qdev-properties.c @@ -15,6 +15,7 @@ #include "chardev/char.h" #include "qemu/uuid.h" #include "qemu/units.h" +#include "qemu/cutils.h" void qdev_prop_set_after_realize(DeviceState *dev, const char *name, Error **errp) @@ -578,6 +579,94 @@ const PropertyInfo qdev_prop_macaddr = { .set = set_mac, }; +/* --- Reserved Region --- */ + +/* + * accepted syntax version: >>> >>> "version" feels redundant. Suggest to capitalize "Accepted". >>> + * :: + * where low/high addresses are uint64_t in hexadecimal + * and type is an unsigned integer in decimal + */ +static void get_reserved_region(Object *obj, Visitor *v, const char *name, +void *opaque, Error **errp) +{ +DeviceState *dev = DEVICE(obj); +Property *prop
Re: [PATCH v4 1/5] qdev: Introduce DEFINE_PROP_RESERVED_REGION
Auger Eric writes: > Hi Markus, > > On 6/23/20 5:13 PM, Markus Armbruster wrote: >> Eric Auger writes: >> >>> Introduce a new property defining a reserved region: >>> ::. >>> >>> This will be used to encode reserved IOVA regions. >>> >>> For instance, in virtio-iommu use case, reserved IOVA regions >>> will be passed by the machine code to the virtio-iommu-pci >>> device (an array of those). The type of the reserved region >>> will match the virtio_iommu_probe_resv_mem subtype value: >>> - VIRTIO_IOMMU_RESV_MEM_T_RESERVED (0) >>> - VIRTIO_IOMMU_RESV_MEM_T_MSI (1) >>> >>> on PC/Q35 machine, this will be used to inform the >>> virtio-iommu-pci device it should bypass the MSI region. >>> The reserved region will be: 0xfee0:0xfeef:1. >>> >>> On ARM, we can declare the ITS MSI doorbell as an MSI >>> region to prevent MSIs from being mapped on guest side. >>> >>> Signed-off-by: Eric Auger >>> Reviewed-by: Markus Armbruster >>> >>> --- >>> >>> v3 -> v4: >>> - use ':' instead of commas as separators. >>> - rearrange error messages >>> - check snprintf returned value >>> - dared to keep Markus' R-b despite those changes >>> --- >>> include/exec/memory.h| 6 +++ >>> include/hw/qdev-properties.h | 3 ++ >>> include/qemu/typedefs.h | 1 + >>> hw/core/qdev-properties.c| 89 >>> 4 files changed, 99 insertions(+) >>> >>> diff --git a/include/exec/memory.h b/include/exec/memory.h >>> index 7207025bd4..d7a53b96cc 100644 >>> --- a/include/exec/memory.h >>> +++ b/include/exec/memory.h >>> @@ -51,6 +51,12 @@ extern bool global_dirty_log; >>> >>> typedef struct MemoryRegionOps MemoryRegionOps; >>> >>> +struct ReservedRegion { >>> +hwaddr low; >>> +hwaddr high; >>> +unsigned int type; >> >> Suggest to s/unsigned int/unsigned/. >> >>> +}; >>> + >>> typedef struct IOMMUTLBEntry IOMMUTLBEntry; >>> >>> /* See address_space_translate: bit 0 is read, bit 1 is write. */ >>> diff --git a/include/hw/qdev-properties.h b/include/hw/qdev-properties.h >>> index 5252bb6b1a..95d0e7201d 100644 >>> --- a/include/hw/qdev-properties.h >>> +++ b/include/hw/qdev-properties.h >>> @@ -19,6 +19,7 @@ extern const PropertyInfo qdev_prop_string; >>> extern const PropertyInfo qdev_prop_chr; >>> extern const PropertyInfo qdev_prop_tpm; >>> extern const PropertyInfo qdev_prop_macaddr; >>> +extern const PropertyInfo qdev_prop_reserved_region; >>> extern const PropertyInfo qdev_prop_on_off_auto; >>> extern const PropertyInfo qdev_prop_multifd_compression; >>> extern const PropertyInfo qdev_prop_losttickpolicy; >>> @@ -184,6 +185,8 @@ extern const PropertyInfo qdev_prop_pcie_link_width; >>> DEFINE_PROP(_n, _s, _f, qdev_prop_drive_iothread, BlockBackend *) >>> #define DEFINE_PROP_MACADDR(_n, _s, _f) \ >>> DEFINE_PROP(_n, _s, _f, qdev_prop_macaddr, MACAddr) >>> +#define DEFINE_PROP_RESERVED_REGION(_n, _s, _f) \ >>> +DEFINE_PROP(_n, _s, _f, qdev_prop_reserved_region, ReservedRegion) >>> #define DEFINE_PROP_ON_OFF_AUTO(_n, _s, _f, _d) \ >>> DEFINE_PROP_SIGNED(_n, _s, _f, _d, qdev_prop_on_off_auto, OnOffAuto) >>> #define DEFINE_PROP_MULTIFD_COMPRESSION(_n, _s, _f, _d) \ >>> diff --git a/include/qemu/typedefs.h b/include/qemu/typedefs.h >>> index ce4a78b687..15f5047bf1 100644 >>> --- a/include/qemu/typedefs.h >>> +++ b/include/qemu/typedefs.h >>> @@ -58,6 +58,7 @@ typedef struct ISABus ISABus; >>> typedef struct ISADevice ISADevice; >>> typedef struct IsaDma IsaDma; >>> typedef struct MACAddr MACAddr; >>> +typedef struct ReservedRegion ReservedRegion; >>> typedef struct MachineClass MachineClass; >>> typedef struct MachineState MachineState; >>> typedef struct MemoryListener MemoryListener; >>> diff --git a/hw/core/qdev-properties.c b/hw/core/qdev-properties.c >>> index ead35d7ffd..193d0d95f9 100644 >>> --- a/hw/core/qdev-properties.c >>> +++ b/hw/core/qdev-properties.c >>> @@ -15,6 +15,7 @@ >>> #include "chardev/char.h" >>> #include "qemu/uuid.h" >>> #include "qemu/units.h" >>> +#include "qemu/cutils.h" >>> >>> void qdev_prop_set_after_realize(DeviceState *dev, const char *name, >>>Error **errp) >>> @@ -578,6 +579,94 @@ const PropertyInfo qdev_prop_macaddr = { >>> .set = set_mac, >>> }; >>> >>> +/* --- Reserved Region --- */ >>> + >>> +/* >>> + * accepted syntax version: >> >> "version" feels redundant. Suggest to capitalize "Accepted". >> >>> + * :: >>> + * where low/high addresses are uint64_t in hexadecimal >>> + * and type is an unsigned integer in decimal >>> + */ >>> +static void get_reserved_region(Object *obj, Visitor *v, const char *name, >>> +void *opaque, Error **errp) >>> +{ >>> +DeviceState *dev = DEVICE(obj); >>> +Property *prop = opaque; >>> +ReservedRegion *rr = qdev_get_prop_ptr(dev, prop); >>> +char buffer[64]; >>> +char *p = buffer; >>> +int rc; >>> + >>> +rc = snprintf(buffer,
Re: [PATCH v4 1/5] qdev: Introduce DEFINE_PROP_RESERVED_REGION
Hi Markus, On 6/23/20 5:13 PM, Markus Armbruster wrote: > Eric Auger writes: > >> Introduce a new property defining a reserved region: >> ::. >> >> This will be used to encode reserved IOVA regions. >> >> For instance, in virtio-iommu use case, reserved IOVA regions >> will be passed by the machine code to the virtio-iommu-pci >> device (an array of those). The type of the reserved region >> will match the virtio_iommu_probe_resv_mem subtype value: >> - VIRTIO_IOMMU_RESV_MEM_T_RESERVED (0) >> - VIRTIO_IOMMU_RESV_MEM_T_MSI (1) >> >> on PC/Q35 machine, this will be used to inform the >> virtio-iommu-pci device it should bypass the MSI region. >> The reserved region will be: 0xfee0:0xfeef:1. >> >> On ARM, we can declare the ITS MSI doorbell as an MSI >> region to prevent MSIs from being mapped on guest side. >> >> Signed-off-by: Eric Auger >> Reviewed-by: Markus Armbruster >> >> --- >> >> v3 -> v4: >> - use ':' instead of commas as separators. >> - rearrange error messages >> - check snprintf returned value >> - dared to keep Markus' R-b despite those changes >> --- >> include/exec/memory.h| 6 +++ >> include/hw/qdev-properties.h | 3 ++ >> include/qemu/typedefs.h | 1 + >> hw/core/qdev-properties.c| 89 >> 4 files changed, 99 insertions(+) >> >> diff --git a/include/exec/memory.h b/include/exec/memory.h >> index 7207025bd4..d7a53b96cc 100644 >> --- a/include/exec/memory.h >> +++ b/include/exec/memory.h >> @@ -51,6 +51,12 @@ extern bool global_dirty_log; >> >> typedef struct MemoryRegionOps MemoryRegionOps; >> >> +struct ReservedRegion { >> +hwaddr low; >> +hwaddr high; >> +unsigned int type; > > Suggest to s/unsigned int/unsigned/. > >> +}; >> + >> typedef struct IOMMUTLBEntry IOMMUTLBEntry; >> >> /* See address_space_translate: bit 0 is read, bit 1 is write. */ >> diff --git a/include/hw/qdev-properties.h b/include/hw/qdev-properties.h >> index 5252bb6b1a..95d0e7201d 100644 >> --- a/include/hw/qdev-properties.h >> +++ b/include/hw/qdev-properties.h >> @@ -19,6 +19,7 @@ extern const PropertyInfo qdev_prop_string; >> extern const PropertyInfo qdev_prop_chr; >> extern const PropertyInfo qdev_prop_tpm; >> extern const PropertyInfo qdev_prop_macaddr; >> +extern const PropertyInfo qdev_prop_reserved_region; >> extern const PropertyInfo qdev_prop_on_off_auto; >> extern const PropertyInfo qdev_prop_multifd_compression; >> extern const PropertyInfo qdev_prop_losttickpolicy; >> @@ -184,6 +185,8 @@ extern const PropertyInfo qdev_prop_pcie_link_width; >> DEFINE_PROP(_n, _s, _f, qdev_prop_drive_iothread, BlockBackend *) >> #define DEFINE_PROP_MACADDR(_n, _s, _f) \ >> DEFINE_PROP(_n, _s, _f, qdev_prop_macaddr, MACAddr) >> +#define DEFINE_PROP_RESERVED_REGION(_n, _s, _f) \ >> +DEFINE_PROP(_n, _s, _f, qdev_prop_reserved_region, ReservedRegion) >> #define DEFINE_PROP_ON_OFF_AUTO(_n, _s, _f, _d) \ >> DEFINE_PROP_SIGNED(_n, _s, _f, _d, qdev_prop_on_off_auto, OnOffAuto) >> #define DEFINE_PROP_MULTIFD_COMPRESSION(_n, _s, _f, _d) \ >> diff --git a/include/qemu/typedefs.h b/include/qemu/typedefs.h >> index ce4a78b687..15f5047bf1 100644 >> --- a/include/qemu/typedefs.h >> +++ b/include/qemu/typedefs.h >> @@ -58,6 +58,7 @@ typedef struct ISABus ISABus; >> typedef struct ISADevice ISADevice; >> typedef struct IsaDma IsaDma; >> typedef struct MACAddr MACAddr; >> +typedef struct ReservedRegion ReservedRegion; >> typedef struct MachineClass MachineClass; >> typedef struct MachineState MachineState; >> typedef struct MemoryListener MemoryListener; >> diff --git a/hw/core/qdev-properties.c b/hw/core/qdev-properties.c >> index ead35d7ffd..193d0d95f9 100644 >> --- a/hw/core/qdev-properties.c >> +++ b/hw/core/qdev-properties.c >> @@ -15,6 +15,7 @@ >> #include "chardev/char.h" >> #include "qemu/uuid.h" >> #include "qemu/units.h" >> +#include "qemu/cutils.h" >> >> void qdev_prop_set_after_realize(DeviceState *dev, const char *name, >>Error **errp) >> @@ -578,6 +579,94 @@ const PropertyInfo qdev_prop_macaddr = { >> .set = set_mac, >> }; >> >> +/* --- Reserved Region --- */ >> + >> +/* >> + * accepted syntax version: > > "version" feels redundant. Suggest to capitalize "Accepted". > >> + * :: >> + * where low/high addresses are uint64_t in hexadecimal >> + * and type is an unsigned integer in decimal >> + */ >> +static void get_reserved_region(Object *obj, Visitor *v, const char *name, >> +void *opaque, Error **errp) >> +{ >> +DeviceState *dev = DEVICE(obj); >> +Property *prop = opaque; >> +ReservedRegion *rr = qdev_get_prop_ptr(dev, prop); >> +char buffer[64]; >> +char *p = buffer; >> +int rc; >> + >> +rc = snprintf(buffer, sizeof(buffer), "0x%"PRIx64":0x%"PRIx64":%u", >> + rr->low, rr->high, rr->type); >> +assert(rc < sizeof(buffer)); >> + >> +
Re: [PATCH v4 1/5] qdev: Introduce DEFINE_PROP_RESERVED_REGION
Eric Auger writes: > Introduce a new property defining a reserved region: > ::. > > This will be used to encode reserved IOVA regions. > > For instance, in virtio-iommu use case, reserved IOVA regions > will be passed by the machine code to the virtio-iommu-pci > device (an array of those). The type of the reserved region > will match the virtio_iommu_probe_resv_mem subtype value: > - VIRTIO_IOMMU_RESV_MEM_T_RESERVED (0) > - VIRTIO_IOMMU_RESV_MEM_T_MSI (1) > > on PC/Q35 machine, this will be used to inform the > virtio-iommu-pci device it should bypass the MSI region. > The reserved region will be: 0xfee0:0xfeef:1. > > On ARM, we can declare the ITS MSI doorbell as an MSI > region to prevent MSIs from being mapped on guest side. > > Signed-off-by: Eric Auger > Reviewed-by: Markus Armbruster > > --- > > v3 -> v4: > - use ':' instead of commas as separators. > - rearrange error messages > - check snprintf returned value > - dared to keep Markus' R-b despite those changes > --- > include/exec/memory.h| 6 +++ > include/hw/qdev-properties.h | 3 ++ > include/qemu/typedefs.h | 1 + > hw/core/qdev-properties.c| 89 > 4 files changed, 99 insertions(+) > > diff --git a/include/exec/memory.h b/include/exec/memory.h > index 7207025bd4..d7a53b96cc 100644 > --- a/include/exec/memory.h > +++ b/include/exec/memory.h > @@ -51,6 +51,12 @@ extern bool global_dirty_log; > > typedef struct MemoryRegionOps MemoryRegionOps; > > +struct ReservedRegion { > +hwaddr low; > +hwaddr high; > +unsigned int type; Suggest to s/unsigned int/unsigned/. > +}; > + > typedef struct IOMMUTLBEntry IOMMUTLBEntry; > > /* See address_space_translate: bit 0 is read, bit 1 is write. */ > diff --git a/include/hw/qdev-properties.h b/include/hw/qdev-properties.h > index 5252bb6b1a..95d0e7201d 100644 > --- a/include/hw/qdev-properties.h > +++ b/include/hw/qdev-properties.h > @@ -19,6 +19,7 @@ extern const PropertyInfo qdev_prop_string; > extern const PropertyInfo qdev_prop_chr; > extern const PropertyInfo qdev_prop_tpm; > extern const PropertyInfo qdev_prop_macaddr; > +extern const PropertyInfo qdev_prop_reserved_region; > extern const PropertyInfo qdev_prop_on_off_auto; > extern const PropertyInfo qdev_prop_multifd_compression; > extern const PropertyInfo qdev_prop_losttickpolicy; > @@ -184,6 +185,8 @@ extern const PropertyInfo qdev_prop_pcie_link_width; > DEFINE_PROP(_n, _s, _f, qdev_prop_drive_iothread, BlockBackend *) > #define DEFINE_PROP_MACADDR(_n, _s, _f) \ > DEFINE_PROP(_n, _s, _f, qdev_prop_macaddr, MACAddr) > +#define DEFINE_PROP_RESERVED_REGION(_n, _s, _f) \ > +DEFINE_PROP(_n, _s, _f, qdev_prop_reserved_region, ReservedRegion) > #define DEFINE_PROP_ON_OFF_AUTO(_n, _s, _f, _d) \ > DEFINE_PROP_SIGNED(_n, _s, _f, _d, qdev_prop_on_off_auto, OnOffAuto) > #define DEFINE_PROP_MULTIFD_COMPRESSION(_n, _s, _f, _d) \ > diff --git a/include/qemu/typedefs.h b/include/qemu/typedefs.h > index ce4a78b687..15f5047bf1 100644 > --- a/include/qemu/typedefs.h > +++ b/include/qemu/typedefs.h > @@ -58,6 +58,7 @@ typedef struct ISABus ISABus; > typedef struct ISADevice ISADevice; > typedef struct IsaDma IsaDma; > typedef struct MACAddr MACAddr; > +typedef struct ReservedRegion ReservedRegion; > typedef struct MachineClass MachineClass; > typedef struct MachineState MachineState; > typedef struct MemoryListener MemoryListener; > diff --git a/hw/core/qdev-properties.c b/hw/core/qdev-properties.c > index ead35d7ffd..193d0d95f9 100644 > --- a/hw/core/qdev-properties.c > +++ b/hw/core/qdev-properties.c > @@ -15,6 +15,7 @@ > #include "chardev/char.h" > #include "qemu/uuid.h" > #include "qemu/units.h" > +#include "qemu/cutils.h" > > void qdev_prop_set_after_realize(DeviceState *dev, const char *name, >Error **errp) > @@ -578,6 +579,94 @@ const PropertyInfo qdev_prop_macaddr = { > .set = set_mac, > }; > > +/* --- Reserved Region --- */ > + > +/* > + * accepted syntax version: "version" feels redundant. Suggest to capitalize "Accepted". > + * :: > + * where low/high addresses are uint64_t in hexadecimal > + * and type is an unsigned integer in decimal > + */ > +static void get_reserved_region(Object *obj, Visitor *v, const char *name, > +void *opaque, Error **errp) > +{ > +DeviceState *dev = DEVICE(obj); > +Property *prop = opaque; > +ReservedRegion *rr = qdev_get_prop_ptr(dev, prop); > +char buffer[64]; > +char *p = buffer; > +int rc; > + > +rc = snprintf(buffer, sizeof(buffer), "0x%"PRIx64":0x%"PRIx64":%u", > + rr->low, rr->high, rr->type); > +assert(rc < sizeof(buffer)); > + > +visit_type_str(v, name, , errp); > +} > + > +static void set_reserved_region(Object *obj, Visitor *v, const char *name, > +void *opaque, Error **errp) > +{ > +DeviceState