[Qemu-devel] [PATCH v2] arm: change vendor ID for virtio-mmio

[Qemu-devel] [PATCH v2] arm: change vendor ID for virtio-mmio

[Michael S. Tsirkin]

ACPI spec 5.0 allows the use of PCI vendor IDs.

Since we have one for virtio, it seems neater to use that
rather than LNRO. For the device ID, use 103F which is a legacy ID that
isn't used in virtio PCI spec - seems to make sense since virtio-mmio is
a legacy device but we don't know the correct device type.

Guests should probably match everything in the range 1000-103F
(just like legacy pci drivers do) which will allow us to pass in the
actual ID in the future if we want to.

Signed-off-by: Michael S. Tsirkin <mst@redhat.com>
---
 hw/arm/virt-acpi-build.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/hw/arm/virt-acpi-build.c b/hw/arm/virt-acpi-build.c
index f365140..dea61ba 100644
--- a/hw/arm/virt-acpi-build.c
+++ b/hw/arm/virt-acpi-build.c
@@ -145,7 +145,7 @@ static void acpi_dsdt_add_virtio(Aml *scope,
 
     for (i = 0; i < num; i++) {
         Aml *dev = aml_device("VR%02u", i);
-        aml_append(dev, aml_name_decl("_HID", aml_string("LNRO0005")));
+        aml_append(dev, aml_name_decl("_HID", aml_string("1AF4103F")));
         aml_append(dev, aml_name_decl("_UID", aml_int(i)));
 
         Aml *crs = aml_resource_template();
-- 
MST

Re: [Qemu-devel] [PATCH v2] arm: change vendor ID for virtio-mmio

[Shannon Zhao]

On 2015/7/30 3:16, Michael S. Tsirkin wrote:
> ACPI spec 5.0 allows the use of PCI vendor IDs.
> 
But virtio-mmio is not a PCI device, it's a platform device.
Why do we drop the previous way using "QEMUXXXX"? Something I missed?

> Since we have one for virtio, it seems neater to use that
> rather than LNRO. For the device ID, use 103F which is a legacy ID that
> isn't used in virtio PCI spec - seems to make sense since virtio-mmio is
> a legacy device but we don't know the correct device type.
> 
> Guests should probably match everything in the range 1000-103F
> (just like legacy pci drivers do) which will allow us to pass in the
> actual ID in the future if we want to.
> 
> Signed-off-by: Michael S. Tsirkin <mst@redhat.com>
> ---
>  hw/arm/virt-acpi-build.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/hw/arm/virt-acpi-build.c b/hw/arm/virt-acpi-build.c
> index f365140..dea61ba 100644
> --- a/hw/arm/virt-acpi-build.c
> +++ b/hw/arm/virt-acpi-build.c
> @@ -145,7 +145,7 @@ static void acpi_dsdt_add_virtio(Aml *scope,
>  
>      for (i = 0; i < num; i++) {
>          Aml *dev = aml_device("VR%02u", i);
> -        aml_append(dev, aml_name_decl("_HID", aml_string("LNRO0005")));
> +        aml_append(dev, aml_name_decl("_HID", aml_string("1AF4103F")));
>          aml_append(dev, aml_name_decl("_UID", aml_int(i)));
>  
>          Aml *crs = aml_resource_template();
> 

-- 
Shannon

Re: [Qemu-devel] [PATCH v2] arm: change vendor ID for virtio-mmio

[Michael S. Tsirkin]

On Thu, Jul 30, 2015 at 09:23:20AM +0800, Shannon Zhao wrote:
> 
> 
> On 2015/7/30 3:16, Michael S. Tsirkin wrote:
> > ACPI spec 5.0 allows the use of PCI vendor IDs.
> > 
> But virtio-mmio is not a PCI device, it's a platform device.

Yes. ACPI spec 5.0 says:

	A valid PNP ID must be of the form "AAA####" where A is an uppercase
	letter and # is a hex digit. A valid ACPI ID must be of the form
	"NNNN####" where N is an uppercase letter or a digit ('0'-'9') and # is
	a hex digit. This specification reserves the string "ACPI" for use only
	with devices defined herein.

	It further reserves all strings representing 4 HEX digits for
	exclusive use with PCI-assigned Vendor IDs.

The second paragraph means if PCI SIG assigned you an ID, you
can use that without need to register it with ASWG.


> Why do we drop the previous way using "QEMUXXXX"? Something I missed?

So that guests that bind to this interface will work fine with non QEMU
implementations of virtio-mmio.

It's just playing nice with others.

We could have done something similar to pvpanic as well, except we
didn't and guests using the QEMU prefix have been released,
so we have to keep using that.

> > Since we have one for virtio, it seems neater to use that
> > rather than LNRO. For the device ID, use 103F which is a legacy ID that
> > isn't used in virtio PCI spec - seems to make sense since virtio-mmio is
> > a legacy device but we don't know the correct device type.
> > 
> > Guests should probably match everything in the range 1000-103F
> > (just like legacy pci drivers do) which will allow us to pass in the
> > actual ID in the future if we want to.
> > 
> > Signed-off-by: Michael S. Tsirkin <mst@redhat.com>
> > ---
> >  hw/arm/virt-acpi-build.c | 2 +-
> >  1 file changed, 1 insertion(+), 1 deletion(-)
> > 
> > diff --git a/hw/arm/virt-acpi-build.c b/hw/arm/virt-acpi-build.c
> > index f365140..dea61ba 100644
> > --- a/hw/arm/virt-acpi-build.c
> > +++ b/hw/arm/virt-acpi-build.c
> > @@ -145,7 +145,7 @@ static void acpi_dsdt_add_virtio(Aml *scope,
> >  
> >      for (i = 0; i < num; i++) {
> >          Aml *dev = aml_device("VR%02u", i);
> > -        aml_append(dev, aml_name_decl("_HID", aml_string("LNRO0005")));
> > +        aml_append(dev, aml_name_decl("_HID", aml_string("1AF4103F")));
> >          aml_append(dev, aml_name_decl("_UID", aml_int(i)));
> >  
> >          Aml *crs = aml_resource_template();
> > 
> 
> -- 
> Shannon

Re: [Qemu-devel] [PATCH v2] arm: change vendor ID for virtio-mmio

[Shannon Zhao]

On 2015/7/30 16:04, Michael S. Tsirkin wrote:
> On Thu, Jul 30, 2015 at 09:23:20AM +0800, Shannon Zhao wrote:
>>
>>
>> On 2015/7/30 3:16, Michael S. Tsirkin wrote:
>>> ACPI spec 5.0 allows the use of PCI vendor IDs.
>>>
>> But virtio-mmio is not a PCI device, it's a platform device.
> 
> Yes. ACPI spec 5.0 says:
> 
> 	A valid PNP ID must be of the form "AAA####" where A is an uppercase
> 	letter and # is a hex digit. A valid ACPI ID must be of the form
> 	"NNNN####" where N is an uppercase letter or a digit ('0'-'9') and # is
> 	a hex digit. This specification reserves the string "ACPI" for use only
> 	with devices defined herein.
> 
> 	It further reserves all strings representing 4 HEX digits for
> 	exclusive use with PCI-assigned Vendor IDs.
> 
> The second paragraph means if PCI SIG assigned you an ID, you
> can use that without need to register it with ASWG.
> 
> 
>> Why do we drop the previous way using "QEMUXXXX"? Something I missed?
> 
> So that guests that bind to this interface will work fine with non QEMU
> implementations of virtio-mmio.
> 

I think kernel driver supports multiple IDs. If they don't want to
"QEMUXXXX" as ACPI ID, it's free to add a new one like below.

+static const struct acpi_device_id virtio_mmio_acpi_match[] = {
+       { "QEMU0005", },
+       { "1AF4103F", },
+	{ }
+};
+MODULE_DEVICE_TABLE(acpi, virtio_mmio_acpi_match);

> It's just playing nice with others.
> 
> We could have done something similar to pvpanic as well, except we
> didn't and guests using the QEMU prefix have been released,
> so we have to keep using that.
> 
>>> Since we have one for virtio, it seems neater to use that
>>> rather than LNRO. For the device ID, use 103F which is a legacy ID that
>>> isn't used in virtio PCI spec - seems to make sense since virtio-mmio is
>>> a legacy device but we don't know the correct device type.
>>>
>>> Guests should probably match everything in the range 1000-103F
>>> (just like legacy pci drivers do) which will allow us to pass in the
>>> actual ID in the future if we want to.
>>>
>>> Signed-off-by: Michael S. Tsirkin <mst@redhat.com>
>>> ---
>>>  hw/arm/virt-acpi-build.c | 2 +-
>>>  1 file changed, 1 insertion(+), 1 deletion(-)
>>>
>>> diff --git a/hw/arm/virt-acpi-build.c b/hw/arm/virt-acpi-build.c
>>> index f365140..dea61ba 100644
>>> --- a/hw/arm/virt-acpi-build.c
>>> +++ b/hw/arm/virt-acpi-build.c
>>> @@ -145,7 +145,7 @@ static void acpi_dsdt_add_virtio(Aml *scope,
>>>  
>>>      for (i = 0; i < num; i++) {
>>>          Aml *dev = aml_device("VR%02u", i);
>>> -        aml_append(dev, aml_name_decl("_HID", aml_string("LNRO0005")));
>>> +        aml_append(dev, aml_name_decl("_HID", aml_string("1AF4103F")));
>>>          aml_append(dev, aml_name_decl("_UID", aml_int(i)));
>>>  
>>>          Aml *crs = aml_resource_template();
>>>
>>
>> -- 
>> Shannon
> 
> .
> 

-- 
Shannon

Re: [Qemu-devel] [PATCH v2] arm: change vendor ID for virtio-mmio

[Michael S. Tsirkin]

On Thu, Jul 30, 2015 at 05:21:51PM +0800, Shannon Zhao wrote:
> 
> 
> On 2015/7/30 16:04, Michael S. Tsirkin wrote:
> > On Thu, Jul 30, 2015 at 09:23:20AM +0800, Shannon Zhao wrote:
> >>
> >>
> >> On 2015/7/30 3:16, Michael S. Tsirkin wrote:
> >>> ACPI spec 5.0 allows the use of PCI vendor IDs.
> >>>
> >> But virtio-mmio is not a PCI device, it's a platform device.
> > 
> > Yes. ACPI spec 5.0 says:
> > 
> > 	A valid PNP ID must be of the form "AAA####" where A is an uppercase
> > 	letter and # is a hex digit. A valid ACPI ID must be of the form
> > 	"NNNN####" where N is an uppercase letter or a digit ('0'-'9') and # is
> > 	a hex digit. This specification reserves the string "ACPI" for use only
> > 	with devices defined herein.
> > 
> > 	It further reserves all strings representing 4 HEX digits for
> > 	exclusive use with PCI-assigned Vendor IDs.
> > 
> > The second paragraph means if PCI SIG assigned you an ID, you
> > can use that without need to register it with ASWG.
> > 
> > 
> >> Why do we drop the previous way using "QEMUXXXX"? Something I missed?
> > 
> > So that guests that bind to this interface will work fine with non QEMU
> > implementations of virtio-mmio.
> > 
> 
> I think kernel driver supports multiple IDs. If they don't want to
> "QEMUXXXX" as ACPI ID, it's free to add a new one like below.
> 
> +static const struct acpi_device_id virtio_mmio_acpi_match[] = {
> +       { "QEMU0005", },
> +       { "1AF4103F", },
> +	{ }
> +};
> +MODULE_DEVICE_TABLE(acpi, virtio_mmio_acpi_match);

Yes but that won't work with existing disto kernels.

> > It's just playing nice with others.
> > 
> > We could have done something similar to pvpanic as well, except we
> > didn't and guests using the QEMU prefix have been released,
> > so we have to keep using that.
> > 
> >>> Since we have one for virtio, it seems neater to use that
> >>> rather than LNRO. For the device ID, use 103F which is a legacy ID that
> >>> isn't used in virtio PCI spec - seems to make sense since virtio-mmio is
> >>> a legacy device but we don't know the correct device type.
> >>>
> >>> Guests should probably match everything in the range 1000-103F
> >>> (just like legacy pci drivers do) which will allow us to pass in the
> >>> actual ID in the future if we want to.
> >>>
> >>> Signed-off-by: Michael S. Tsirkin <mst@redhat.com>
> >>> ---
> >>>  hw/arm/virt-acpi-build.c | 2 +-
> >>>  1 file changed, 1 insertion(+), 1 deletion(-)
> >>>
> >>> diff --git a/hw/arm/virt-acpi-build.c b/hw/arm/virt-acpi-build.c
> >>> index f365140..dea61ba 100644
> >>> --- a/hw/arm/virt-acpi-build.c
> >>> +++ b/hw/arm/virt-acpi-build.c
> >>> @@ -145,7 +145,7 @@ static void acpi_dsdt_add_virtio(Aml *scope,
> >>>  
> >>>      for (i = 0; i < num; i++) {
> >>>          Aml *dev = aml_device("VR%02u", i);
> >>> -        aml_append(dev, aml_name_decl("_HID", aml_string("LNRO0005")));
> >>> +        aml_append(dev, aml_name_decl("_HID", aml_string("1AF4103F")));
> >>>          aml_append(dev, aml_name_decl("_UID", aml_int(i)));
> >>>  
> >>>          Aml *crs = aml_resource_template();
> >>>
> >>
> >> -- 
> >> Shannon
> > 
> > .
> > 
> 
> -- 
> Shannon

Re: [Qemu-devel] [PATCH v2] arm: change vendor ID for virtio-mmio

[Peter Maydell]

On 30 July 2015 at 09:04, Michael S. Tsirkin <mst@redhat.com> wrote:
> On Thu, Jul 30, 2015 at 09:23:20AM +0800, Shannon Zhao wrote:
>>
>> Why do we drop the previous way using "QEMUXXXX"? Something I missed?
>
> So that guests that bind to this interface will work fine with non QEMU
> implementations of virtio-mmio.

I don't understand this sentence. If there are pre-existing
non-QEMU virtio-mmio implementations, then they're using
LNRO0005, and we should use it too. If there are going to
be implementations of virtio-mmio in future, then they will
use whatever identifier we pick here. Either way, we get
interoperability. I don't see any difference between our
saying "the ID for virtio-mmio is QEMU0005" and saying
"the ID for virtio-mmio is 1AF4103F".

(The latter seems unnecessarily opaque to me, to be honest.
At least an ID string QEMUxxxx gives you a clue where to
look for who owns the thing.)

Note also that strictly you don't mean "non-QEMU implementations
of virtio-mmio", you mean "non-QEMU implementations of the
ACPI tables". The hardware implementation of virtio-mmio
doesn't care at all about the ACPI ID. (In fact the most
plausible other-implementation would be UEFI using its
own (hard-coded) ACPI tables on top of a QEMU vexpress-a15
model or something similar.)

-- PMM

Re: [Qemu-devel] [PATCH v2] arm: change vendor ID for virtio-mmio

[G Gregory]

On 30 July 2015 at 10:24, Peter Maydell <peter.maydell@linaro.org> wrote:
> On 30 July 2015 at 09:04, Michael S. Tsirkin <mst@redhat.com> wrote:
>> On Thu, Jul 30, 2015 at 09:23:20AM +0800, Shannon Zhao wrote:
>>>
>>> Why do we drop the previous way using "QEMUXXXX"? Something I missed?
>>
>> So that guests that bind to this interface will work fine with non QEMU
>> implementations of virtio-mmio.
>
> I don't understand this sentence. If there are pre-existing
> non-QEMU virtio-mmio implementations, then they're using
> LNRO0005, and we should use it too.

The only one I have come across is the ARM FVP model, and it happens
that I chose the ID and maintain the tables for that so I can change
it.

Graeme

Re: [Qemu-devel] [PATCH v2] arm: change vendor ID for virtio-mmio

[G Gregory]

On 30 July 2015 at 10:37, G Gregory <graeme.gregory@linaro.org> wrote:
> On 30 July 2015 at 10:24, Peter Maydell <peter.maydell@linaro.org> wrote:
>> On 30 July 2015 at 09:04, Michael S. Tsirkin <mst@redhat.com> wrote:
>>> On Thu, Jul 30, 2015 at 09:23:20AM +0800, Shannon Zhao wrote:
>>>>
>>>> Why do we drop the previous way using "QEMUXXXX"? Something I missed?
>>>
>>> So that guests that bind to this interface will work fine with non QEMU
>>> implementations of virtio-mmio.
>>
>> I don't understand this sentence. If there are pre-existing
>> non-QEMU virtio-mmio implementations, then they're using
>> LNRO0005, and we should use it too.
>
> The only one I have come across is the ARM FVP model, and it happens
> that I chose the ID and maintain the tables for that so I can change
> it.
>
In fact, I would just add

Name (_HID, "QEMUXXXX")
Name (_HID, "1AF4103F")

To the tables so tables work with old (internal) kernels and new!

Graeme

Re: [Qemu-devel] [PATCH v2] arm: change vendor ID for virtio-mmio

[Michael S. Tsirkin]

On Thu, Jul 30, 2015 at 10:24:11AM +0100, Peter Maydell wrote:
> On 30 July 2015 at 09:04, Michael S. Tsirkin <mst@redhat.com> wrote:
> > On Thu, Jul 30, 2015 at 09:23:20AM +0800, Shannon Zhao wrote:
> >>
> >> Why do we drop the previous way using "QEMUXXXX"? Something I missed?
> >
> > So that guests that bind to this interface will work fine with non QEMU
> > implementations of virtio-mmio.
> 
> I don't understand this sentence. If there are pre-existing
> non-QEMU virtio-mmio implementations, then they're using
> LNRO0005, and we should use it too. If there are going to
> be implementations of virtio-mmio in future, then they will
> use whatever identifier we pick here. Either way, we get
> interoperability. I don't see any difference between our
> saying "the ID for virtio-mmio is QEMU0005" and saying
> "the ID for virtio-mmio is 1AF4103F".

I agree. It's just that 1AF4 is already reserved for virtio.

> (The latter seems unnecessarily opaque to me, to be honest.
> At least an ID string QEMUxxxx gives you a clue where to
> look for who owns the thing.)

Well - if one looks in the ACPI spec, that says if ID uses numbers, then
one has to find the vendor from PCI SIG, and that has a database mapping
IDs to vendors.

> 
> Note also that strictly you don't mean "non-QEMU implementations
> of virtio-mmio", you mean "non-QEMU implementations of the
> ACPI tables".

Yes.

> The hardware implementation of virtio-mmio
> doesn't care at all about the ACPI ID. (In fact the most
> plausible other-implementation would be UEFI using its
> own (hard-coded) ACPI tables on top of a QEMU vexpress-a15
> model or something similar.)
> 
> -- PMM

Re: [Qemu-devel] [PATCH v2] arm: change vendor ID for virtio-mmio

[Peter Maydell]

On 29 July 2015 at 20:16, Michael S. Tsirkin <mst@redhat.com> wrote:
> ACPI spec 5.0 allows the use of PCI vendor IDs.
>
> Since we have one for virtio, it seems neater to use that
> rather than LNRO. For the device ID, use 103F which is a legacy ID that
> isn't used in virtio PCI spec - seems to make sense since virtio-mmio is
> a legacy device but we don't know the correct device type.
>
> Guests should probably match everything in the range 1000-103F
> (just like legacy pci drivers do) which will allow us to pass in the
> actual ID in the future if we want to.
>
> Signed-off-by: Michael S. Tsirkin <mst@redhat.com>
> ---
>  hw/arm/virt-acpi-build.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/hw/arm/virt-acpi-build.c b/hw/arm/virt-acpi-build.c
> index f365140..dea61ba 100644
> --- a/hw/arm/virt-acpi-build.c
> +++ b/hw/arm/virt-acpi-build.c
> @@ -145,7 +145,7 @@ static void acpi_dsdt_add_virtio(Aml *scope,
>
>      for (i = 0; i < num; i++) {
>          Aml *dev = aml_device("VR%02u", i);
> -        aml_append(dev, aml_name_decl("_HID", aml_string("LNRO0005")));
> +        aml_append(dev, aml_name_decl("_HID", aml_string("1AF4103F")));
>          aml_append(dev, aml_name_decl("_UID", aml_int(i)));

So, I've just checked, and I believe that the kernel that RedHat
are shipping in their RHEL7 dev preview for AArch64 (and probably
thus also the Fedora/Centos one) includes a patch which adds
ACPI support to the virtio-mmio driver using the LNRO0005 ID string.

This to me suggests that we should just stick with that ID,
rather than changing to QEMUxxxx, the hex one based on the PCI
vendor ID, or anything else.

We're obviously under no obligation to make life easy for people
who ship kernels full of patches that haven't gone upstream yet,
but in this case there doesn't seem to me to be any benefit to
QEMU from picking an ID string that would break compatibility...

[The kernel I checked was the one in
https://git.centos.org/sources/kernel-aarch64/c7-aarch64/c589ab77889df6d93dbe817c373080631ab3275b
which despite the filename is actually an 80MB .tar.xz archive,
as pointed to by
https://git.centos.org/blob/rpms!kernel-aarch64/910dbce5f13419d68002f58e67ee6e762a93a425/.kernel-aarch64.metadata
]

thanks
-- PMM

Re: [Qemu-devel] [PATCH v2] arm: change vendor ID for virtio-mmio

[Andrew Jones]

On Fri, Jul 31, 2015 at 01:37:44PM +0100, Peter Maydell wrote:
> On 29 July 2015 at 20:16, Michael S. Tsirkin <mst@redhat.com> wrote:
> > ACPI spec 5.0 allows the use of PCI vendor IDs.
> >
> > Since we have one for virtio, it seems neater to use that
> > rather than LNRO. For the device ID, use 103F which is a legacy ID that
> > isn't used in virtio PCI spec - seems to make sense since virtio-mmio is
> > a legacy device but we don't know the correct device type.
> >
> > Guests should probably match everything in the range 1000-103F
> > (just like legacy pci drivers do) which will allow us to pass in the
> > actual ID in the future if we want to.
> >
> > Signed-off-by: Michael S. Tsirkin <mst@redhat.com>
> > ---
> >  hw/arm/virt-acpi-build.c | 2 +-
> >  1 file changed, 1 insertion(+), 1 deletion(-)
> >
> > diff --git a/hw/arm/virt-acpi-build.c b/hw/arm/virt-acpi-build.c
> > index f365140..dea61ba 100644
> > --- a/hw/arm/virt-acpi-build.c
> > +++ b/hw/arm/virt-acpi-build.c
> > @@ -145,7 +145,7 @@ static void acpi_dsdt_add_virtio(Aml *scope,
> >
> >      for (i = 0; i < num; i++) {
> >          Aml *dev = aml_device("VR%02u", i);
> > -        aml_append(dev, aml_name_decl("_HID", aml_string("LNRO0005")));
> > +        aml_append(dev, aml_name_decl("_HID", aml_string("1AF4103F")));
> >          aml_append(dev, aml_name_decl("_UID", aml_int(i)));
> 
> So, I've just checked, and I believe that the kernel that RedHat
> are shipping in their RHEL7 dev preview for AArch64 (and probably
> thus also the Fedora/Centos one) includes a patch which adds
> ACPI support to the virtio-mmio driver using the LNRO0005 ID string.

Yes, we have https://lkml.org/lkml/2015/7/28/216 in the RHELSA kernel.
Although, while that kernel is floating around already, it hasn't
really been released, particularly not for virt use cases. So we could
change it (although that change needs to be soon).

> 
> This to me suggests that we should just stick with that ID,
> rather than changing to QEMUxxxx, the hex one based on the PCI
> vendor ID, or anything else.
> 
> We're obviously under no obligation to make life easy for people
> who ship kernels full of patches that haven't gone upstream yet,
> but in this case there doesn't seem to me to be any benefit to
> QEMU from picking an ID string that would break compatibility...

I'll remain abstained on this debate, but if the consensus is to
change it, then let me know, and I'll handle the changes on the
RHELSA side.

Thanks,
drew

> 
> [The kernel I checked was the one in
> https://git.centos.org/sources/kernel-aarch64/c7-aarch64/c589ab77889df6d93dbe817c373080631ab3275b
> which despite the filename is actually an 80MB .tar.xz archive,
> as pointed to by
> https://git.centos.org/blob/rpms!kernel-aarch64/910dbce5f13419d68002f58e67ee6e762a93a425/.kernel-aarch64.metadata
> ]
> 
> thanks
> -- PMM
>