| Message ID | 20200611151209.22547-2-eric.auger@redhat.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | VIRTIO-IOMMU probe request support and MSI bypass on ARM | expand |
Eric Auger <eric.auger@redhat.com> writes: > Introduce a new property defining a reserved region: > <low address>, <high address>, <type>. > > 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: 0xfee00000, 0xfeefffff, 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 <eric.auger@redhat.com> > > --- > > v11 -> v12: > - rename into DEFINE_PROP_RESERVED_REGION > - do not use g_strsplit anymore, use endptr instead > - remove 0x references > --- > 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 3e00cdbbfa..3ee8224fa7 100644 > --- a/include/exec/memory.h > +++ b/include/exec/memory.h > @@ -57,6 +57,12 @@ struct MemoryRegionMmio { > CPUWriteMemoryFunc *write[3]; > }; > > +struct ReservedRegion { > + hwaddr low; > + hwaddr high; > + unsigned int type; > +}; > + > 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 f161604fb6..03bf850a7e 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; > @@ -183,6 +184,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 ecf3cde26c..85c4f891f4 100644 > --- a/include/qemu/typedefs.h > +++ b/include/qemu/typedefs.h > @@ -59,6 +59,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 cc924815da..15b84adbee 100644 > --- a/hw/core/qdev-properties.c > +++ b/hw/core/qdev-properties.c > @@ -14,6 +14,7 @@ > #include "qapi/visitor.h" > #include "chardev/char.h" > #include "qemu/uuid.h" > +#include "qemu/cutils.h" > > void qdev_prop_set_after_realize(DeviceState *dev, const char *name, > Error **errp) > @@ -577,6 +578,94 @@ const PropertyInfo qdev_prop_macaddr = { > .set = set_mac, > }; > > +/* --- Reserved Region --- */ > + > +/* > + * accepted syntax version: > + * <low address>,<high address>,<type> > + * 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; > + > + snprintf(buffer, sizeof(buffer), "0x%"PRIx64",0x%"PRIx64",%u", > + rr->low, rr->high, rr->type); Matches existing practice in other getters. Nevertheless, I'd suggest something like n = snprintf(buffer, sizeof(buffer), ...); assert(n < sizeof(buffer); > + > + visit_type_str(v, name, &p, errp); > +} > + > +static void set_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); > + Error *local_err = NULL; > + const char *endptr; > + char *str; > + int ret; > + > + if (dev->realized) { > + qdev_prop_set_after_realize(dev, name, errp); > + return; > + } > + > + visit_type_str(v, name, &str, &local_err); > + if (local_err) { > + error_propagate(errp, local_err); > + return; > + } > + > + ret = qemu_strtou64(str, &endptr, 16, &rr->low); > + if (ret) { > + error_setg(errp, "Failed to decode reserved region low addr"); > + error_append_hint(errp, > + "should be an address in hexadecimal\n"); Comes out like this: qemu-system-x86_64: -device ...: Failed to decode reserved region low addr should be an address in hexadecimal I'd capitalize the other way, to get qemu-system-x86_64: -device ...: failed to decode reserved region low addr Should be an address in hexadecimal Note: output is made up; I failed at figuring out how to use the new property. An example in PATCH 4's commit message might help. Since the error message fails to mention @name, the user is left guessing unless "-device ..." contains just one reserved region parameter. What about something like this: error_setg(errp, "start address of reserved region '%s'" " must be a hexadecimal integer", name); No need to mess around with error_append_hint() then. Same for the other error messages. > + goto out; > + } > + if (*endptr != ',') { > + goto separator_error; > + } > + > + ret = qemu_strtou64(endptr + 1, &endptr, 16, &rr->high); > + if (ret) { > + error_setg(errp, "Failed to decode reserved region high addr"); > + error_append_hint(errp, > + "should be an address in hexadecimal\n"); > + goto out; > + } > + if (*endptr != ',') { > + goto separator_error; > + } > + > + ret = qemu_strtoui(endptr + 1, &endptr, 10, &rr->type); > + if (ret) { > + error_setg(errp, "Failed to decode reserved region type"); > + error_append_hint(errp, "should be an unsigned integer in decimal\n"); > + } I dimly remember discussing the wisdom of numeric type here, dig, dig, ..., aha: Subject: Re: [PATCH for-5.0 v11 12/20] qapi: Introduce DEFINE_PROP_INTERVAL Date: Fri, 13 Dec 2019 11:03:02 +0100 Message-ID: <87y2vg4k6h.fsf@dusky.pond.sub.org> >> So the "label" part of "<low address>,<high address>,label" is a number? > yes it is. >> >> Is a number appropriate for your use case, or would an enum be better? > I think a number is OK. There might be other types of reserved regions > in the future. Also if we want to allow somebody else to reuse that > property in another context, I would rather leave it open? I'd prioritize the user interface over possible reuse (which might never happen). Mind, I'm not telling you using numbers is a bad user interface. In general, enums are nicer, but I don't know enough about this particular case. > + goto out; > + > +separator_error: > + error_setg(errp, "reserved region fields must be separated with commas"); I'm not sure de-duplicating this error message is worth the extra goto. > +out: > + g_free(str); > + return; > +} > + > +const PropertyInfo qdev_prop_reserved_region = { > + .name = "reserved_region", > + .description = "Reserved Region, example: 0xFEE00000,0xFEEFFFFF,0", > + .get = get_reserved_region, > + .set = set_reserved_region, > +}; > + > /* --- on/off/auto --- */ > > const PropertyInfo qdev_prop_on_off_auto = { Can't find anything that's actually wrong, so Reviewed-by: Markus Armbruster <armbru@redhat.com>
Hi Markus, On 6/22/20 1:22 PM, Markus Armbruster wrote: > Eric Auger <eric.auger@redhat.com> writes: > >> Introduce a new property defining a reserved region: >> <low address>, <high address>, <type>. >> >> 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: 0xfee00000, 0xfeefffff, 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 <eric.auger@redhat.com> >> >> --- >> >> v11 -> v12: >> - rename into DEFINE_PROP_RESERVED_REGION >> - do not use g_strsplit anymore, use endptr instead >> - remove 0x references >> --- >> 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 3e00cdbbfa..3ee8224fa7 100644 >> --- a/include/exec/memory.h >> +++ b/include/exec/memory.h >> @@ -57,6 +57,12 @@ struct MemoryRegionMmio { >> CPUWriteMemoryFunc *write[3]; >> }; >> >> +struct ReservedRegion { >> + hwaddr low; >> + hwaddr high; >> + unsigned int type; >> +}; >> + >> 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 f161604fb6..03bf850a7e 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; >> @@ -183,6 +184,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 ecf3cde26c..85c4f891f4 100644 >> --- a/include/qemu/typedefs.h >> +++ b/include/qemu/typedefs.h >> @@ -59,6 +59,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 cc924815da..15b84adbee 100644 >> --- a/hw/core/qdev-properties.c >> +++ b/hw/core/qdev-properties.c >> @@ -14,6 +14,7 @@ >> #include "qapi/visitor.h" >> #include "chardev/char.h" >> #include "qemu/uuid.h" >> +#include "qemu/cutils.h" >> >> void qdev_prop_set_after_realize(DeviceState *dev, const char *name, >> Error **errp) >> @@ -577,6 +578,94 @@ const PropertyInfo qdev_prop_macaddr = { >> .set = set_mac, >> }; >> >> +/* --- Reserved Region --- */ >> + >> +/* >> + * accepted syntax version: >> + * <low address>,<high address>,<type> >> + * 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; >> + >> + snprintf(buffer, sizeof(buffer), "0x%"PRIx64",0x%"PRIx64",%u", >> + rr->low, rr->high, rr->type); > > Matches existing practice in other getters. Nevertheless, I'd suggest > something like > > n = snprintf(buffer, sizeof(buffer), ...); > assert(n < sizeof(buffer); OK > >> + >> + visit_type_str(v, name, &p, errp); >> +} >> + >> +static void set_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); >> + Error *local_err = NULL; >> + const char *endptr; >> + char *str; >> + int ret; >> + >> + if (dev->realized) { >> + qdev_prop_set_after_realize(dev, name, errp); >> + return; >> + } >> + >> + visit_type_str(v, name, &str, &local_err); >> + if (local_err) { >> + error_propagate(errp, local_err); >> + return; >> + } >> + >> + ret = qemu_strtou64(str, &endptr, 16, &rr->low); >> + if (ret) { >> + error_setg(errp, "Failed to decode reserved region low addr"); >> + error_append_hint(errp, >> + "should be an address in hexadecimal\n"); > > Comes out like this: > > qemu-system-x86_64: -device ...: Failed to decode reserved region low addr > should be an address in hexadecimal > > I'd capitalize the other way, to get > > qemu-system-x86_64: -device ...: failed to decode reserved region low addr > Should be an address in hexadecimal > > Note: output is made up; I failed at figuring out how to use the new > property. An example in PATCH 4's commit message might help. OK I will add one example. In practice in the virtio-iommu case the property is not really meant to be passed by the end-user but should be set by the machine code. However I have just tested from the cmd line and it looks using commas as separators is a bad idea because it collides with ',' separating properties. So if you're OK I will change the comma into ':'. > > Since the error message fails to mention @name, the user is left > guessing unless "-device ..." contains just one reserved region > parameter. > > What about something like this: > > error_setg(errp, "start address of reserved region '%s'" > " must be a hexadecimal integer", > name); Sure. Given the fact the property is named reserved-region[n], this may be simplified into error_setg(errp, "start address of '%s'" " must be a hexadecimal integer", > name); > > No need to mess around with error_append_hint() then. OK > > Same for the other error messages. > >> + goto out; >> + } >> + if (*endptr != ',') { >> + goto separator_error; >> + } >> + >> + ret = qemu_strtou64(endptr + 1, &endptr, 16, &rr->high); >> + if (ret) { >> + error_setg(errp, "Failed to decode reserved region high addr"); >> + error_append_hint(errp, >> + "should be an address in hexadecimal\n"); >> + goto out; >> + } >> + if (*endptr != ',') { >> + goto separator_error; >> + } >> + >> + ret = qemu_strtoui(endptr + 1, &endptr, 10, &rr->type); >> + if (ret) { >> + error_setg(errp, "Failed to decode reserved region type"); >> + error_append_hint(errp, "should be an unsigned integer in decimal\n"); >> + } > > I dimly remember discussing the wisdom of numeric type here, dig, dig, > ..., aha: > > Subject: Re: [PATCH for-5.0 v11 12/20] qapi: Introduce DEFINE_PROP_INTERVAL > Date: Fri, 13 Dec 2019 11:03:02 +0100 > Message-ID: <87y2vg4k6h.fsf@dusky.pond.sub.org> > > >> So the "label" part of "<low address>,<high address>,label" is a number? > > yes it is. > >> > >> Is a number appropriate for your use case, or would an enum be better? > > I think a number is OK. There might be other types of reserved regions > > in the future. Also if we want to allow somebody else to reuse that > > property in another context, I would rather leave it open? > > I'd prioritize the user interface over possible reuse (which might never > happen). Mind, I'm not telling you using numbers is a bad user > interface. In general, enums are nicer, but I don't know enough about > this particular case. Yep I remember too ;-) I left as it was because I think this property could be used for other use cases. > >> + goto out; >> + >> +separator_error: >> + error_setg(errp, "reserved region fields must be separated with commas"); > > I'm not sure de-duplicating this error message is worth the extra goto. > >> +out: >> + g_free(str); >> + return; >> +} >> + >> +const PropertyInfo qdev_prop_reserved_region = { >> + .name = "reserved_region", >> + .description = "Reserved Region, example: 0xFEE00000,0xFEEFFFFF,0", >> + .get = get_reserved_region, >> + .set = set_reserved_region, >> +}; >> + >> /* --- on/off/auto --- */ >> >> const PropertyInfo qdev_prop_on_off_auto = { > > Can't find anything that's actually wrong, so > > Reviewed-by: Markus Armbruster <armbru@redhat.com> Thanks Eric >
Auger Eric <eric.auger@redhat.com> writes: > Hi Markus, > > On 6/22/20 1:22 PM, Markus Armbruster wrote: >> Eric Auger <eric.auger@redhat.com> writes: >> >>> Introduce a new property defining a reserved region: >>> <low address>, <high address>, <type>. >>> >>> 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: 0xfee00000, 0xfeefffff, 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 <eric.auger@redhat.com> [...] >>> diff --git a/hw/core/qdev-properties.c b/hw/core/qdev-properties.c >>> index cc924815da..15b84adbee 100644 >>> --- a/hw/core/qdev-properties.c >>> +++ b/hw/core/qdev-properties.c [...] >>> +static void set_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); >>> + Error *local_err = NULL; >>> + const char *endptr; >>> + char *str; >>> + int ret; >>> + >>> + if (dev->realized) { >>> + qdev_prop_set_after_realize(dev, name, errp); >>> + return; >>> + } >>> + >>> + visit_type_str(v, name, &str, &local_err); >>> + if (local_err) { >>> + error_propagate(errp, local_err); >>> + return; >>> + } >>> + >>> + ret = qemu_strtou64(str, &endptr, 16, &rr->low); >>> + if (ret) { >>> + error_setg(errp, "Failed to decode reserved region low addr"); >>> + error_append_hint(errp, >>> + "should be an address in hexadecimal\n"); >> >> Comes out like this: >> >> qemu-system-x86_64: -device ...: Failed to decode reserved region low addr >> should be an address in hexadecimal >> >> I'd capitalize the other way, to get >> >> qemu-system-x86_64: -device ...: failed to decode reserved region low addr >> Should be an address in hexadecimal >> >> Note: output is made up; I failed at figuring out how to use the new >> property. An example in PATCH 4's commit message might help. > OK I will add one example. In practice in the virtio-iommu case the > property is not really meant to be passed by the end-user but should be > set by the machine code. However I have just tested from the cmd line > and it looks using commas as separators is a bad idea because it > collides with ',' separating properties. So if you're OK I will change > the comma into ':'. Please do. [...]
Auger Eric <eric.auger@redhat.com> writes: > Hi Markus, > > On 6/22/20 1:22 PM, Markus Armbruster wrote: >> Eric Auger <eric.auger@redhat.com> writes: >> >>> Introduce a new property defining a reserved region: >>> <low address>, <high address>, <type>. [...] >> I dimly remember discussing the wisdom of numeric type here, dig, dig, >> ..., aha: >> >> Subject: Re: [PATCH for-5.0 v11 12/20] qapi: Introduce DEFINE_PROP_INTERVAL >> Date: Fri, 13 Dec 2019 11:03:02 +0100 >> Message-ID: <87y2vg4k6h.fsf@dusky.pond.sub.org> >> >> >> So the "label" part of "<low address>,<high address>,label" is a number? >> > yes it is. >> >> >> >> Is a number appropriate for your use case, or would an enum be better? >> > I think a number is OK. There might be other types of reserved regions >> > in the future. Also if we want to allow somebody else to reuse that >> > property in another context, I would rather leave it open? >> >> I'd prioritize the user interface over possible reuse (which might never >> happen). Mind, I'm not telling you using numbers is a bad user >> interface. In general, enums are nicer, but I don't know enough about >> this particular case. > Yep I remember too ;-) I left as it was because I think this property > could be used for other use cases. YAGNI :) A string would work, too, wouldn't it? [...]
Hi Markus, On 6/23/20 5:15 PM, Markus Armbruster wrote: > Auger Eric <eric.auger@redhat.com> writes: > >> Hi Markus, >> >> On 6/22/20 1:22 PM, Markus Armbruster wrote: >>> Eric Auger <eric.auger@redhat.com> writes: >>> >>>> Introduce a new property defining a reserved region: >>>> <low address>, <high address>, <type>. > [...] >>> I dimly remember discussing the wisdom of numeric type here, dig, dig, >>> ..., aha: >>> >>> Subject: Re: [PATCH for-5.0 v11 12/20] qapi: Introduce DEFINE_PROP_INTERVAL >>> Date: Fri, 13 Dec 2019 11:03:02 +0100 >>> Message-ID: <87y2vg4k6h.fsf@dusky.pond.sub.org> >>> >>> >> So the "label" part of "<low address>,<high address>,label" is a number? >>> > yes it is. >>> >> >>> >> Is a number appropriate for your use case, or would an enum be better? >>> > I think a number is OK. There might be other types of reserved regions >>> > in the future. Also if we want to allow somebody else to reuse that >>> > property in another context, I would rather leave it open? >>> >>> I'd prioritize the user interface over possible reuse (which might never >>> happen). Mind, I'm not telling you using numbers is a bad user >>> interface. In general, enums are nicer, but I don't know enough about >>> this particular case. >> Yep I remember too ;-) I left as it was because I think this property >> could be used for other use cases. > > YAGNI :) > > A string would work, too, wouldn't it? :-) Eric > > [...] > >
diff --git a/include/exec/memory.h b/include/exec/memory.h index 3e00cdbbfa..3ee8224fa7 100644 --- a/include/exec/memory.h +++ b/include/exec/memory.h @@ -57,6 +57,12 @@ struct MemoryRegionMmio { CPUWriteMemoryFunc *write[3]; }; +struct ReservedRegion { + hwaddr low; + hwaddr high; + unsigned int type; +}; + 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 f161604fb6..03bf850a7e 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; @@ -183,6 +184,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 ecf3cde26c..85c4f891f4 100644 --- a/include/qemu/typedefs.h +++ b/include/qemu/typedefs.h @@ -59,6 +59,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 cc924815da..15b84adbee 100644 --- a/hw/core/qdev-properties.c +++ b/hw/core/qdev-properties.c @@ -14,6 +14,7 @@ #include "qapi/visitor.h" #include "chardev/char.h" #include "qemu/uuid.h" +#include "qemu/cutils.h" void qdev_prop_set_after_realize(DeviceState *dev, const char *name, Error **errp) @@ -577,6 +578,94 @@ const PropertyInfo qdev_prop_macaddr = { .set = set_mac, }; +/* --- Reserved Region --- */ + +/* + * accepted syntax version: + * <low address>,<high address>,<type> + * 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; + + snprintf(buffer, sizeof(buffer), "0x%"PRIx64",0x%"PRIx64",%u", + rr->low, rr->high, rr->type); + + visit_type_str(v, name, &p, errp); +} + +static void set_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); + Error *local_err = NULL; + const char *endptr; + char *str; + int ret; + + if (dev->realized) { + qdev_prop_set_after_realize(dev, name, errp); + return; + } + + visit_type_str(v, name, &str, &local_err); + if (local_err) { + error_propagate(errp, local_err); + return; + } + + ret = qemu_strtou64(str, &endptr, 16, &rr->low); + if (ret) { + error_setg(errp, "Failed to decode reserved region low addr"); + error_append_hint(errp, + "should be an address in hexadecimal\n"); + goto out; + } + if (*endptr != ',') { + goto separator_error; + } + + ret = qemu_strtou64(endptr + 1, &endptr, 16, &rr->high); + if (ret) { + error_setg(errp, "Failed to decode reserved region high addr"); + error_append_hint(errp, + "should be an address in hexadecimal\n"); + goto out; + } + if (*endptr != ',') { + goto separator_error; + } + + ret = qemu_strtoui(endptr + 1, &endptr, 10, &rr->type); + if (ret) { + error_setg(errp, "Failed to decode reserved region type"); + error_append_hint(errp, "should be an unsigned integer in decimal\n"); + } + goto out; + +separator_error: + error_setg(errp, "reserved region fields must be separated with commas"); +out: + g_free(str); + return; +} + +const PropertyInfo qdev_prop_reserved_region = { + .name = "reserved_region", + .description = "Reserved Region, example: 0xFEE00000,0xFEEFFFFF,0", + .get = get_reserved_region, + .set = set_reserved_region, +}; + /* --- on/off/auto --- */ const PropertyInfo qdev_prop_on_off_auto = {
Introduce a new property defining a reserved region: <low address>, <high address>, <type>. 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: 0xfee00000, 0xfeefffff, 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 <eric.auger@redhat.com> --- v11 -> v12: - rename into DEFINE_PROP_RESERVED_REGION - do not use g_strsplit anymore, use endptr instead - remove 0x references --- 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(+)