Re: [PATCH] virtio_bt: Fix alignment in configuration struct

Re: [PATCH] virtio_bt: Fix alignment in configuration struct

[Michael S. Tsirkin]

On Mon, Aug 08, 2022 at 12:11:52AM +0200, Igor Skalkin wrote:
> According to specification [1], "For the device-specific configuration
> space, the driver MUST use 8 bit wide accesses for 8 bit wide fields,
> 16 bit wide and aligned accesses for 16 bit wide fields and 32 bit wide
> and aligned accesses for 32 and 64 bit wide fields.".
> 
> Current version of the configuration structure:
> 
>     struct virtio_bt_config {
>         __u8  type;
>         __u16 vendor;
>         __u16 msft_opcode;
>     } __attribute__((packed));
> 
> has both 16bit fields non-aligned.
> 
> This commit fixes it.
> 
> [1] https://docs.oasis-open.org/virtio/virtio/v1.1/virtio-v1.1.pdf
> 
> Signed-off-by: Igor Skalkin <Igor.Skalkin@opensynergy.com>

This is all true enough, but the problem is
1. changing uapi like this can't be done, will break userspace
2. the driver has more issues and no one seems to want to
   maintain it. 
I posted a patch "Bluetooth: virtio_bt: mark broken" and intend
to merge it for this release.


> ---
>  include/uapi/linux/virtio_bt.h | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/include/uapi/linux/virtio_bt.h b/include/uapi/linux/virtio_bt.h
> index a7bd48daa9a9..adc03709cc4f 100644
> --- a/include/uapi/linux/virtio_bt.h
> +++ b/include/uapi/linux/virtio_bt.h
> @@ -23,9 +23,9 @@ enum virtio_bt_config_vendor {
>  };
>  
>  struct virtio_bt_config {
> -	__u8  type;
>  	__u16 vendor;
>  	__u16 msft_opcode;
> +	__u8  type;
>  } __attribute__((packed));
>  
>  #endif /* _UAPI_LINUX_VIRTIO_BT_H */
> -- 
> 2.34.1

Re: [PATCH] virtio_bt: Fix alignment in configuration struct

[Michael S. Tsirkin]

On Mon, Aug 08, 2022 at 02:04:43PM +0200, Igor Skalkin wrote:
> On 8/8/22 01:00, Michael S. Tsirkin wrote:
> 
>     On Mon, Aug 08, 2022 at 12:11:52AM +0200, Igor Skalkin wrote:
> 
>         According to specification [1], "For the device-specific configuration
>         space, the driver MUST use 8 bit wide accesses for 8 bit wide fields,
>         16 bit wide and aligned accesses for 16 bit wide fields and 32 bit wide
>         and aligned accesses for 32 and 64 bit wide fields.".
> 
>         Current version of the configuration structure:
> 
>             struct virtio_bt_config {
>                 __u8  type;
>                 __u16 vendor;
>                 __u16 msft_opcode;
>             } __attribute__((packed));
> 
>         has both 16bit fields non-aligned.
> 
>         This commit fixes it.
> 
>         [1] https://ddec1-0-en-ctp.trendmicro.com:443/wis/clicktime/v1/query?url=https%3a%2f%2fdocs.oasis%2dopen.org%2fvirtio%2fvirtio%2fv1.1%2fvirtio%2dv1.1.pdf&umid=d1786ace-e8ea-40e8-9665-96c0949174e5&auth=53c7c7de28b92dfd96e93d9dd61a23e634d2fbec-39b15885ceebe9fda9357320aec1ccbac416a470
> 
>         Signed-off-by: Igor Skalkin <Igor.Skalkin@opensynergy.com>
> 
>     This is all true enough, but the problem is
>     1. changing uapi like this can't be done, will break userspace
>     2. the driver has more issues and no one seems to want to
>        maintain it.
>     I posted a patch "Bluetooth: virtio_bt: mark broken" and intend
>     to merge it for this release.
> 
> This is very sad. We already use this driver in our projects.

Really?  Can you step up to maintain it? Then we can fix the issues
and it won't be broken.

> Our virtio bluetooth device has two backends - HCI_USER socket backend for one
> platform and uart backend for the other, and works well (after applying your
> "[PATCH] Bluetooth: virtio_bt: fix device remove") patch, so this "device
> removal" problem can probably be considered solved .

Can you post a Tested-by tag for that?

I need to go back and review it, I think I saw some issues but must be
fixable.

> We could help with the rest of the problems you listed that can be solved
> (specification, QEMU support).
> And the only problem that is difficult to solve (because of the need to change
> UAPI header files) is just this one with unaligned configuration fields.
> At the moment, it does not reproduce, because without VIRTIO_BT_F_VND_HCI
> (Indicates vendor command support) feature negotiated, the driver does not
> read the non-aligned configuration fields.


Hmm. So how about this:
- add a new feature flag
- add new aligned format
- mark the old memory reserved in the spec


> So, what would you advise us to do? Continuing to use the "marked broken"
> driver, start writing a specification for a new from scratch, better one?
> Or is there any way to bring this one back to life?

If someone is prepared to work on this we can bring it back.



> 
> 
>         ---
>          include/uapi/linux/virtio_bt.h | 2 +-
>          1 file changed, 1 insertion(+), 1 deletion(-)
> 
>         diff --git a/include/uapi/linux/virtio_bt.h b/include/uapi/linux/virtio_bt.h
>         index a7bd48daa9a9..adc03709cc4f 100644
>         --- a/include/uapi/linux/virtio_bt.h
>         +++ b/include/uapi/linux/virtio_bt.h
>         @@ -23,9 +23,9 @@ enum virtio_bt_config_vendor {
>          };
> 
>          struct virtio_bt_config {
>         -       __u8  type;
>                 __u16 vendor;
>                 __u16 msft_opcode;
>         +       __u8  type;
>          } __attribute__((packed));
> 
>          #endif /* _UAPI_LINUX_VIRTIO_BT_H */
>         --
>         2.34.1
> 
> --
> 
> Best regards,
> 
> Igor Skalkin
> Software Engineer
> 
> OpenSynergy GmbH
> Rotherstr. 20, 10245 Berlin
> 
> igor.skalkin@opensynergy.com
> www.opensynergy.com
> 
> registered: Amtsgericht Charlottenburg, HRB 108616B
> General Management: Rolf Morich, Stefaan Sonck Thiebaut
> 
> 
> Please mind our privacy notice pursuant to Art. 13 GDPR. // Unsere Hinweise zum
> Datenschutz gem. Art. 13 DSGVO finden Sie hier.

Re: [PATCH] virtio_bt: Fix alignment in configuration struct

[Michael S. Tsirkin]

On Mon, Aug 08, 2022 at 08:16:11AM -0400, Michael S. Tsirkin wrote:
> On Mon, Aug 08, 2022 at 02:04:43PM +0200, Igor Skalkin wrote:
> > On 8/8/22 01:00, Michael S. Tsirkin wrote:
> > 
> >     On Mon, Aug 08, 2022 at 12:11:52AM +0200, Igor Skalkin wrote:
> > 
> >         According to specification [1], "For the device-specific configuration
> >         space, the driver MUST use 8 bit wide accesses for 8 bit wide fields,
> >         16 bit wide and aligned accesses for 16 bit wide fields and 32 bit wide
> >         and aligned accesses for 32 and 64 bit wide fields.".
> > 
> >         Current version of the configuration structure:
> > 
> >             struct virtio_bt_config {
> >                 __u8  type;
> >                 __u16 vendor;
> >                 __u16 msft_opcode;
> >             } __attribute__((packed));
> > 
> >         has both 16bit fields non-aligned.
> > 
> >         This commit fixes it.
> > 
> >         [1] https://ddec1-0-en-ctp.trendmicro.com:443/wis/clicktime/v1/query?url=https%3a%2f%2fdocs.oasis%2dopen.org%2fvirtio%2fvirtio%2fv1.1%2fvirtio%2dv1.1.pdf&umid=d1786ace-e8ea-40e8-9665-96c0949174e5&auth=53c7c7de28b92dfd96e93d9dd61a23e634d2fbec-39b15885ceebe9fda9357320aec1ccbac416a470
> > 
> >         Signed-off-by: Igor Skalkin <Igor.Skalkin@opensynergy.com>
> > 
> >     This is all true enough, but the problem is
> >     1. changing uapi like this can't be done, will break userspace
> >     2. the driver has more issues and no one seems to want to
> >        maintain it.
> >     I posted a patch "Bluetooth: virtio_bt: mark broken" and intend
> >     to merge it for this release.
> > 
> > This is very sad. We already use this driver in our projects.
> 
> Really?  Can you step up to maintain it? Then we can fix the issues
> and it won't be broken.

Just a reminder that I'm waiting for a response on that.
I just don't know enough about bluetooth.

-- 
MST

Re: [PATCH] virtio_bt: Fix alignment in configuration struct

[Luiz Augusto von Dentz]

Hi Michael,

On Thu, Aug 11, 2022 at 1:00 AM Michael S. Tsirkin <mst@redhat.com> wrote:
>
> On Mon, Aug 08, 2022 at 08:16:11AM -0400, Michael S. Tsirkin wrote:
> > On Mon, Aug 08, 2022 at 02:04:43PM +0200, Igor Skalkin wrote:
> > > On 8/8/22 01:00, Michael S. Tsirkin wrote:
> > >
> > >     On Mon, Aug 08, 2022 at 12:11:52AM +0200, Igor Skalkin wrote:
> > >
> > >         According to specification [1], "For the device-specific configuration
> > >         space, the driver MUST use 8 bit wide accesses for 8 bit wide fields,
> > >         16 bit wide and aligned accesses for 16 bit wide fields and 32 bit wide
> > >         and aligned accesses for 32 and 64 bit wide fields.".
> > >
> > >         Current version of the configuration structure:
> > >
> > >             struct virtio_bt_config {
> > >                 __u8  type;
> > >                 __u16 vendor;
> > >                 __u16 msft_opcode;
> > >             } __attribute__((packed));
> > >
> > >         has both 16bit fields non-aligned.
> > >
> > >         This commit fixes it.
> > >
> > >         [1] https://ddec1-0-en-ctp.trendmicro.com:443/wis/clicktime/v1/query?url=https%3a%2f%2fdocs.oasis%2dopen.org%2fvirtio%2fvirtio%2fv1.1%2fvirtio%2dv1.1.pdf&umid=d1786ace-e8ea-40e8-9665-96c0949174e5&auth=53c7c7de28b92dfd96e93d9dd61a23e634d2fbec-39b15885ceebe9fda9357320aec1ccbac416a470
> > >
> > >         Signed-off-by: Igor Skalkin <Igor.Skalkin@opensynergy.com>
> > >
> > >     This is all true enough, but the problem is
> > >     1. changing uapi like this can't be done, will break userspace
> > >     2. the driver has more issues and no one seems to want to
> > >        maintain it.
> > >     I posted a patch "Bluetooth: virtio_bt: mark broken" and intend
> > >     to merge it for this release.
> > >
> > > This is very sad. We already use this driver in our projects.
> >
> > Really?  Can you step up to maintain it? Then we can fix the issues
> > and it won't be broken.
>
> Just a reminder that I'm waiting for a response on that.
> I just don't know enough about bluetooth.

Just a heads up that Marcel is on vacation, he did mention that he had
done some work to update virtio_bt thus why I didn't apply any of the
changes yet.

> --
> MST
>


-- 
Luiz Augusto von Dentz

Re: [PATCH] virtio_bt: Fix alignment in configuration struct

[Michael S. Tsirkin]

On Thu, Aug 11, 2022 at 10:02:31AM -0700, Luiz Augusto von Dentz wrote:
> Hi Michael,
> 
> On Thu, Aug 11, 2022 at 1:00 AM Michael S. Tsirkin <mst@redhat.com> wrote:
> >
> > On Mon, Aug 08, 2022 at 08:16:11AM -0400, Michael S. Tsirkin wrote:
> > > On Mon, Aug 08, 2022 at 02:04:43PM +0200, Igor Skalkin wrote:
> > > > On 8/8/22 01:00, Michael S. Tsirkin wrote:
> > > >
> > > >     On Mon, Aug 08, 2022 at 12:11:52AM +0200, Igor Skalkin wrote:
> > > >
> > > >         According to specification [1], "For the device-specific configuration
> > > >         space, the driver MUST use 8 bit wide accesses for 8 bit wide fields,
> > > >         16 bit wide and aligned accesses for 16 bit wide fields and 32 bit wide
> > > >         and aligned accesses for 32 and 64 bit wide fields.".
> > > >
> > > >         Current version of the configuration structure:
> > > >
> > > >             struct virtio_bt_config {
> > > >                 __u8  type;
> > > >                 __u16 vendor;
> > > >                 __u16 msft_opcode;
> > > >             } __attribute__((packed));
> > > >
> > > >         has both 16bit fields non-aligned.
> > > >
> > > >         This commit fixes it.
> > > >
> > > >         [1] https://ddec1-0-en-ctp.trendmicro.com:443/wis/clicktime/v1/query?url=https%3a%2f%2fdocs.oasis%2dopen.org%2fvirtio%2fvirtio%2fv1.1%2fvirtio%2dv1.1.pdf&umid=d1786ace-e8ea-40e8-9665-96c0949174e5&auth=53c7c7de28b92dfd96e93d9dd61a23e634d2fbec-39b15885ceebe9fda9357320aec1ccbac416a470
> > > >
> > > >         Signed-off-by: Igor Skalkin <Igor.Skalkin@opensynergy.com>
> > > >
> > > >     This is all true enough, but the problem is
> > > >     1. changing uapi like this can't be done, will break userspace
> > > >     2. the driver has more issues and no one seems to want to
> > > >        maintain it.
> > > >     I posted a patch "Bluetooth: virtio_bt: mark broken" and intend
> > > >     to merge it for this release.
> > > >
> > > > This is very sad. We already use this driver in our projects.
> > >
> > > Really?  Can you step up to maintain it? Then we can fix the issues
> > > and it won't be broken.
> >
> > Just a reminder that I'm waiting for a response on that.
> > I just don't know enough about bluetooth.
> 
> Just a heads up that Marcel is on vacation, he did mention that he had
> done some work to update virtio_bt thus why I didn't apply any of the
> changes yet.

Any update? when does Marcel return?

> > --
> > MST
> >
> 
> 
> -- 
> Luiz Augusto von Dentz

Re: [PATCH] virtio_bt: Fix alignment in configuration struct

[Michael S. Tsirkin]

On Tue, Aug 30, 2022 at 09:45:02AM -0400, Michael S. Tsirkin wrote:
> On Thu, Aug 11, 2022 at 10:02:31AM -0700, Luiz Augusto von Dentz wrote:
> > Hi Michael,
> > 
> > On Thu, Aug 11, 2022 at 1:00 AM Michael S. Tsirkin <mst@redhat.com> wrote:
> > >
> > > On Mon, Aug 08, 2022 at 08:16:11AM -0400, Michael S. Tsirkin wrote:
> > > > On Mon, Aug 08, 2022 at 02:04:43PM +0200, Igor Skalkin wrote:
> > > > > On 8/8/22 01:00, Michael S. Tsirkin wrote:
> > > > >
> > > > >     On Mon, Aug 08, 2022 at 12:11:52AM +0200, Igor Skalkin wrote:
> > > > >
> > > > >         According to specification [1], "For the device-specific configuration
> > > > >         space, the driver MUST use 8 bit wide accesses for 8 bit wide fields,
> > > > >         16 bit wide and aligned accesses for 16 bit wide fields and 32 bit wide
> > > > >         and aligned accesses for 32 and 64 bit wide fields.".
> > > > >
> > > > >         Current version of the configuration structure:
> > > > >
> > > > >             struct virtio_bt_config {
> > > > >                 __u8  type;
> > > > >                 __u16 vendor;
> > > > >                 __u16 msft_opcode;
> > > > >             } __attribute__((packed));
> > > > >
> > > > >         has both 16bit fields non-aligned.
> > > > >
> > > > >         This commit fixes it.
> > > > >
> > > > >         [1] https://ddec1-0-en-ctp.trendmicro.com:443/wis/clicktime/v1/query?url=https%3a%2f%2fdocs.oasis%2dopen.org%2fvirtio%2fvirtio%2fv1.1%2fvirtio%2dv1.1.pdf&umid=d1786ace-e8ea-40e8-9665-96c0949174e5&auth=53c7c7de28b92dfd96e93d9dd61a23e634d2fbec-39b15885ceebe9fda9357320aec1ccbac416a470
> > > > >
> > > > >         Signed-off-by: Igor Skalkin <Igor.Skalkin@opensynergy.com>
> > > > >
> > > > >     This is all true enough, but the problem is
> > > > >     1. changing uapi like this can't be done, will break userspace
> > > > >     2. the driver has more issues and no one seems to want to
> > > > >        maintain it.
> > > > >     I posted a patch "Bluetooth: virtio_bt: mark broken" and intend
> > > > >     to merge it for this release.
> > > > >
> > > > > This is very sad. We already use this driver in our projects.
> > > >
> > > > Really?  Can you step up to maintain it? Then we can fix the issues
> > > > and it won't be broken.
> > >
> > > Just a reminder that I'm waiting for a response on that.
> > > I just don't know enough about bluetooth.
> > 
> > Just a heads up that Marcel is on vacation, he did mention that he had
> > done some work to update virtio_bt thus why I didn't apply any of the
> > changes yet.
> 
> Any update? when does Marcel return?


Annnnnnd ... this is falling between the chairs again?
Guys if anyone wants to use this driver it needs a maintainer.

> > > --
> > > MST
> > >
> > 
> > 
> > -- 
> > Luiz Augusto von Dentz

Re: [PATCH] virtio_bt: Fix alignment in configuration struct

[Michael S. Tsirkin]

On Mon, Aug 08, 2022 at 02:04:43PM +0200, Igor Skalkin wrote:
> On 8/8/22 01:00, Michael S. Tsirkin wrote:
> 
>     On Mon, Aug 08, 2022 at 12:11:52AM +0200, Igor Skalkin wrote:
> 
>         According to specification [1], "For the device-specific configuration
>         space, the driver MUST use 8 bit wide accesses for 8 bit wide fields,
>         16 bit wide and aligned accesses for 16 bit wide fields and 32 bit wide
>         and aligned accesses for 32 and 64 bit wide fields.".
> 
>         Current version of the configuration structure:
> 
>             struct virtio_bt_config {
>                 __u8  type;
>                 __u16 vendor;
>                 __u16 msft_opcode;
>             } __attribute__((packed));
> 
>         has both 16bit fields non-aligned.
> 
>         This commit fixes it.
> 
>         [1] https://ddec1-0-en-ctp.trendmicro.com:443/wis/clicktime/v1/query?url=https%3a%2f%2fdocs.oasis%2dopen.org%2fvirtio%2fvirtio%2fv1.1%2fvirtio%2dv1.1.pdf&umid=d1786ace-e8ea-40e8-9665-96c0949174e5&auth=53c7c7de28b92dfd96e93d9dd61a23e634d2fbec-39b15885ceebe9fda9357320aec1ccbac416a470
> 
>         Signed-off-by: Igor Skalkin <Igor.Skalkin@opensynergy.com>
> 
>     This is all true enough, but the problem is
>     1. changing uapi like this can't be done, will break userspace
>     2. the driver has more issues and no one seems to want to
>        maintain it.
>     I posted a patch "Bluetooth: virtio_bt: mark broken" and intend
>     to merge it for this release.
> 
> This is very sad. We already use this driver in our projects.

Ping. If we still have no maintainer I'm marking it broken, users
should at least be warned.


> Our virtio bluetooth device has two backends - HCI_USER socket backend for one
> platform and uart backend for the other, and works well (after applying your
> "[PATCH] Bluetooth: virtio_bt: fix device remove") patch, so this "device
> removal" problem can probably be considered solved .
> We could help with the rest of the problems you listed that can be solved
> (specification, QEMU support).
> And the only problem that is difficult to solve (because of the need to change
> UAPI header files) is just this one with unaligned configuration fields.
> At the moment, it does not reproduce, because without VIRTIO_BT_F_VND_HCI
> (Indicates vendor command support) feature negotiated, the driver does not
> read the non-aligned configuration fields.
> 
> So, what would you advise us to do? Continuing to use the "marked broken"
> driver, start writing a specification for a new from scratch, better one?
> Or is there any way to bring this one back to life?
> 
> 
> 
>         ---
>          include/uapi/linux/virtio_bt.h | 2 +-
>          1 file changed, 1 insertion(+), 1 deletion(-)
> 
>         diff --git a/include/uapi/linux/virtio_bt.h b/include/uapi/linux/virtio_bt.h
>         index a7bd48daa9a9..adc03709cc4f 100644
>         --- a/include/uapi/linux/virtio_bt.h
>         +++ b/include/uapi/linux/virtio_bt.h
>         @@ -23,9 +23,9 @@ enum virtio_bt_config_vendor {
>          };
> 
>          struct virtio_bt_config {
>         -       __u8  type;
>                 __u16 vendor;
>                 __u16 msft_opcode;
>         +       __u8  type;
>          } __attribute__((packed));
> 
>          #endif /* _UAPI_LINUX_VIRTIO_BT_H */
>         --
>         2.34.1
> 
> --
> 
> Best regards,
> 
> Igor Skalkin
> Software Engineer
> 
> OpenSynergy GmbH
> Rotherstr. 20, 10245 Berlin
> 
> igor.skalkin@opensynergy.com
> www.opensynergy.com
> 
> registered: Amtsgericht Charlottenburg, HRB 108616B
> General Management: Rolf Morich, Stefaan Sonck Thiebaut
> 
> 
> Please mind our privacy notice pursuant to Art. 13 GDPR. // Unsere Hinweise zum
> Datenschutz gem. Art. 13 DSGVO finden Sie hier.

Re: [PATCH] virtio_bt: Fix alignment in configuration struct

[Luiz Augusto von Dentz]

Hi Michael,

On Fri, Oct 7, 2022 at 6:03 AM Michael S. Tsirkin <mst@redhat.com> wrote:
>
> On Mon, Aug 08, 2022 at 02:04:43PM +0200, Igor Skalkin wrote:
> > On 8/8/22 01:00, Michael S. Tsirkin wrote:
> >
> >     On Mon, Aug 08, 2022 at 12:11:52AM +0200, Igor Skalkin wrote:
> >
> >         According to specification [1], "For the device-specific configuration
> >         space, the driver MUST use 8 bit wide accesses for 8 bit wide fields,
> >         16 bit wide and aligned accesses for 16 bit wide fields and 32 bit wide
> >         and aligned accesses for 32 and 64 bit wide fields.".
> >
> >         Current version of the configuration structure:
> >
> >             struct virtio_bt_config {
> >                 __u8  type;
> >                 __u16 vendor;
> >                 __u16 msft_opcode;
> >             } __attribute__((packed));
> >
> >         has both 16bit fields non-aligned.
> >
> >         This commit fixes it.
> >
> >         [1] https://ddec1-0-en-ctp.trendmicro.com:443/wis/clicktime/v1/query?url=https%3a%2f%2fdocs.oasis%2dopen.org%2fvirtio%2fvirtio%2fv1.1%2fvirtio%2dv1.1.pdf&umid=d1786ace-e8ea-40e8-9665-96c0949174e5&auth=53c7c7de28b92dfd96e93d9dd61a23e634d2fbec-39b15885ceebe9fda9357320aec1ccbac416a470
> >
> >         Signed-off-by: Igor Skalkin <Igor.Skalkin@opensynergy.com>
> >
> >     This is all true enough, but the problem is
> >     1. changing uapi like this can't be done, will break userspace
> >     2. the driver has more issues and no one seems to want to
> >        maintain it.
> >     I posted a patch "Bluetooth: virtio_bt: mark broken" and intend
> >     to merge it for this release.
> >
> > This is very sad. We already use this driver in our projects.
>
> Ping. If we still have no maintainer I'm marking it broken, users
> should at least be warned.

Please resend.

>
> > Our virtio bluetooth device has two backends - HCI_USER socket backend for one
> > platform and uart backend for the other, and works well (after applying your
> > "[PATCH] Bluetooth: virtio_bt: fix device remove") patch, so this "device
> > removal" problem can probably be considered solved .
> > We could help with the rest of the problems you listed that can be solved
> > (specification, QEMU support).
> > And the only problem that is difficult to solve (because of the need to change
> > UAPI header files) is just this one with unaligned configuration fields.
> > At the moment, it does not reproduce, because without VIRTIO_BT_F_VND_HCI
> > (Indicates vendor command support) feature negotiated, the driver does not
> > read the non-aligned configuration fields.
> >
> > So, what would you advise us to do? Continuing to use the "marked broken"
> > driver, start writing a specification for a new from scratch, better one?
> > Or is there any way to bring this one back to life?
> >
> >
> >
> >         ---
> >          include/uapi/linux/virtio_bt.h | 2 +-
> >          1 file changed, 1 insertion(+), 1 deletion(-)
> >
> >         diff --git a/include/uapi/linux/virtio_bt.h b/include/uapi/linux/virtio_bt.h
> >         index a7bd48daa9a9..adc03709cc4f 100644
> >         --- a/include/uapi/linux/virtio_bt.h
> >         +++ b/include/uapi/linux/virtio_bt.h
> >         @@ -23,9 +23,9 @@ enum virtio_bt_config_vendor {
> >          };
> >
> >          struct virtio_bt_config {
> >         -       __u8  type;
> >                 __u16 vendor;
> >                 __u16 msft_opcode;
> >         +       __u8  type;
> >          } __attribute__((packed));
> >
> >          #endif /* _UAPI_LINUX_VIRTIO_BT_H */
> >         --
> >         2.34.1
> >
> > --
> >
> > Best regards,
> >
> > Igor Skalkin
> > Software Engineer
> >
> > OpenSynergy GmbH
> > Rotherstr. 20, 10245 Berlin
> >
> > igor.skalkin@opensynergy.com
> > www.opensynergy.com
> >
> > registered: Amtsgericht Charlottenburg, HRB 108616B
> > General Management: Rolf Morich, Stefaan Sonck Thiebaut
> >
> >
> > Please mind our privacy notice pursuant to Art. 13 GDPR. // Unsere Hinweise zum
> > Datenschutz gem. Art. 13 DSGVO finden Sie hier.
>


-- 
Luiz Augusto von Dentz

Re: [PATCH] virtio_bt: Fix alignment in configuration struct

[Igor Skalkin]

Hi Luiz,

Current version of this patch is wrong [q]changing uapi like this can't
be done, will break userspace[/q], next version is in process, will be
sent in few days.
Best regards,
Igor
On 10/7/22 21:33, Luiz Augusto von Dentz wrote:
> Hi Michael,
>
> On Fri, Oct 7, 2022 at 6:03 AM Michael S. Tsirkin <mst@redhat.com> wrote:
>>
>> On Mon, Aug 08, 2022 at 02:04:43PM +0200, Igor Skalkin wrote:
>>> On 8/8/22 01:00, Michael S. Tsirkin wrote:
>>>
>>>      On Mon, Aug 08, 2022 at 12:11:52AM +0200, Igor Skalkin wrote:
>>>
>>>          According to specification [1], "For the device-specific configuration
>>>          space, the driver MUST use 8 bit wide accesses for 8 bit wide fields,
>>>          16 bit wide and aligned accesses for 16 bit wide fields and 32 bit wide
>>>          and aligned accesses for 32 and 64 bit wide fields.".
>>>
>>>          Current version of the configuration structure:
>>>
>>>              struct virtio_bt_config {
>>>                  __u8  type;
>>>                  __u16 vendor;
>>>                  __u16 msft_opcode;
>>>              } __attribute__((packed));
>>>
>>>          has both 16bit fields non-aligned.
>>>
>>>          This commit fixes it.
>>>
>>>          [1] https://ddec1-0-en-ctp.trendmicro.com:443/wis/clicktime/v1/query?url=https%3a%2f%2fdocs.oasis%2dopen.org%2fvirtio%2fvirtio%2fv1.1%2fvirtio%2dv1.1.pdf&umid=b1110db2-819d-4f27-b35e-18ac23ce0ab4&auth=53c7c7de28b92dfd96e93d9dd61a23e634d2fbec-2c53002097633a932e7d67b899e6bf6999cdc899
>>>
>>>          Signed-off-by: Igor Skalkin <Igor.Skalkin@opensynergy.com>
>>>
>>>      This is all true enough, but the problem is
>>>      1. changing uapi like this can't be done, will break userspace
>>>      2. the driver has more issues and no one seems to want to
>>>         maintain it.
>>>      I posted a patch "Bluetooth: virtio_bt: mark broken" and intend
>>>      to merge it for this release.
>>>
>>> This is very sad. We already use this driver in our projects.
>>
>> Ping. If we still have no maintainer I'm marking it broken, users
>> should at least be warned.
>
> Please resend.
>
>>
>>> Our virtio bluetooth device has two backends - HCI_USER socket backend for one
>>> platform and uart backend for the other, and works well (after applying your
>>> "[PATCH] Bluetooth: virtio_bt: fix device remove") patch, so this "device
>>> removal" problem can probably be considered solved .
>>> We could help with the rest of the problems you listed that can be solved
>>> (specification, QEMU support).
>>> And the only problem that is difficult to solve (because of the need to change
>>> UAPI header files) is just this one with unaligned configuration fields.
>>> At the moment, it does not reproduce, because without VIRTIO_BT_F_VND_HCI
>>> (Indicates vendor command support) feature negotiated, the driver does not
>>> read the non-aligned configuration fields.
>>>
>>> So, what would you advise us to do? Continuing to use the "marked broken"
>>> driver, start writing a specification for a new from scratch, better one?
>>> Or is there any way to bring this one back to life?
>>>
>>>
>>>
>>>          ---
>>>           include/uapi/linux/virtio_bt.h | 2 +-
>>>           1 file changed, 1 insertion(+), 1 deletion(-)
>>>
>>>          diff --git a/include/uapi/linux/virtio_bt.h b/include/uapi/linux/virtio_bt.h
>>>          index a7bd48daa9a9..adc03709cc4f 100644
>>>          --- a/include/uapi/linux/virtio_bt.h
>>>          +++ b/include/uapi/linux/virtio_bt.h
>>>          @@ -23,9 +23,9 @@ enum virtio_bt_config_vendor {
>>>           };
>>>
>>>           struct virtio_bt_config {
>>>          -       __u8  type;
>>>                  __u16 vendor;
>>>                  __u16 msft_opcode;
>>>          +       __u8  type;
>>>           } __attribute__((packed));
>>>
>>>           #endif /* _UAPI_LINUX_VIRTIO_BT_H */
>>>          --
>>>          2.34.1
>>>
>>> --
>>>
>>> Best regards,
>>>
>>> Igor Skalkin
>>> Software Engineer
>>>
>>> OpenSynergy GmbH
>>> Rotherstr. 20, 10245 Berlin
>>>
>>> igor.skalkin@opensynergy.com
>>> www.opensynergy.com
>>>
>>> registered: Amtsgericht Charlottenburg, HRB 108616B
>>> General Management: Rolf Morich, Stefaan Sonck Thiebaut
>>>
>>>
>>> Please mind our privacy notice pursuant to Art. 13 GDPR. // Unsere Hinweise zum
>>> Datenschutz gem. Art. 13 DSGVO finden Sie hier.
>>
>
>

Please mind our privacy notice<https://www.opensynergy.com/datenschutzerklaerung/privacy-notice-for-business-partners-pursuant-to-article-13-of-the-general-data-protection-regulation-gdpr/> pursuant to Art. 13 GDPR. // Unsere Hinweise zum Datenschutz gem. Art. 13 DSGVO finden Sie hier.<https://www.opensynergy.com/de/datenschutzerklaerung/datenschutzhinweise-fuer-geschaeftspartner-gem-art-13-dsgvo/>

Re: [PATCH] virtio_bt: Fix alignment in configuration struct

[Luiz Augusto von Dentz]

Hi Igor,

On Tue, Oct 11, 2022 at 5:23 AM Igor Skalkin
<igor.skalkin@opensynergy.com> wrote:
>
> Hi Luiz,
>
> Current version of this patch is wrong [q]changing uapi like this can't
> be done, will break userspace[/q], next version is in process, will be
> sent in few days.

Thanks for the update, I will wait for the next version then.

> Best regards,
> Igor
> On 10/7/22 21:33, Luiz Augusto von Dentz wrote:
> > Hi Michael,
> >
> > On Fri, Oct 7, 2022 at 6:03 AM Michael S. Tsirkin <mst@redhat.com> wrote:
> >>
> >> On Mon, Aug 08, 2022 at 02:04:43PM +0200, Igor Skalkin wrote:
> >>> On 8/8/22 01:00, Michael S. Tsirkin wrote:
> >>>
> >>>      On Mon, Aug 08, 2022 at 12:11:52AM +0200, Igor Skalkin wrote:
> >>>
> >>>          According to specification [1], "For the device-specific configuration
> >>>          space, the driver MUST use 8 bit wide accesses for 8 bit wide fields,
> >>>          16 bit wide and aligned accesses for 16 bit wide fields and 32 bit wide
> >>>          and aligned accesses for 32 and 64 bit wide fields.".
> >>>
> >>>          Current version of the configuration structure:
> >>>
> >>>              struct virtio_bt_config {
> >>>                  __u8  type;
> >>>                  __u16 vendor;
> >>>                  __u16 msft_opcode;
> >>>              } __attribute__((packed));
> >>>
> >>>          has both 16bit fields non-aligned.
> >>>
> >>>          This commit fixes it.
> >>>
> >>>          [1] https://ddec1-0-en-ctp.trendmicro.com:443/wis/clicktime/v1/query?url=https%3a%2f%2fdocs.oasis%2dopen.org%2fvirtio%2fvirtio%2fv1.1%2fvirtio%2dv1.1.pdf&umid=b1110db2-819d-4f27-b35e-18ac23ce0ab4&auth=53c7c7de28b92dfd96e93d9dd61a23e634d2fbec-2c53002097633a932e7d67b899e6bf6999cdc899
> >>>
> >>>          Signed-off-by: Igor Skalkin <Igor.Skalkin@opensynergy.com>
> >>>
> >>>      This is all true enough, but the problem is
> >>>      1. changing uapi like this can't be done, will break userspace
> >>>      2. the driver has more issues and no one seems to want to
> >>>         maintain it.
> >>>      I posted a patch "Bluetooth: virtio_bt: mark broken" and intend
> >>>      to merge it for this release.
> >>>
> >>> This is very sad. We already use this driver in our projects.
> >>
> >> Ping. If we still have no maintainer I'm marking it broken, users
> >> should at least be warned.
> >
> > Please resend.
> >
> >>
> >>> Our virtio bluetooth device has two backends - HCI_USER socket backend for one
> >>> platform and uart backend for the other, and works well (after applying your
> >>> "[PATCH] Bluetooth: virtio_bt: fix device remove") patch, so this "device
> >>> removal" problem can probably be considered solved .
> >>> We could help with the rest of the problems you listed that can be solved
> >>> (specification, QEMU support).
> >>> And the only problem that is difficult to solve (because of the need to change
> >>> UAPI header files) is just this one with unaligned configuration fields.
> >>> At the moment, it does not reproduce, because without VIRTIO_BT_F_VND_HCI
> >>> (Indicates vendor command support) feature negotiated, the driver does not
> >>> read the non-aligned configuration fields.
> >>>
> >>> So, what would you advise us to do? Continuing to use the "marked broken"
> >>> driver, start writing a specification for a new from scratch, better one?
> >>> Or is there any way to bring this one back to life?
> >>>
> >>>
> >>>
> >>>          ---
> >>>           include/uapi/linux/virtio_bt.h | 2 +-
> >>>           1 file changed, 1 insertion(+), 1 deletion(-)
> >>>
> >>>          diff --git a/include/uapi/linux/virtio_bt.h b/include/uapi/linux/virtio_bt.h
> >>>          index a7bd48daa9a9..adc03709cc4f 100644
> >>>          --- a/include/uapi/linux/virtio_bt.h
> >>>          +++ b/include/uapi/linux/virtio_bt.h
> >>>          @@ -23,9 +23,9 @@ enum virtio_bt_config_vendor {
> >>>           };
> >>>
> >>>           struct virtio_bt_config {
> >>>          -       __u8  type;
> >>>                  __u16 vendor;
> >>>                  __u16 msft_opcode;
> >>>          +       __u8  type;
> >>>           } __attribute__((packed));
> >>>
> >>>           #endif /* _UAPI_LINUX_VIRTIO_BT_H */
> >>>          --
> >>>          2.34.1
> >>>
> >>> --
> >>>
> >>> Best regards,
> >>>
> >>> Igor Skalkin
> >>> Software Engineer
> >>>
> >>> OpenSynergy GmbH
> >>> Rotherstr. 20, 10245 Berlin
> >>>
> >>> igor.skalkin@opensynergy.com
> >>> www.opensynergy.com
> >>>
> >>> registered: Amtsgericht Charlottenburg, HRB 108616B
> >>> General Management: Rolf Morich, Stefaan Sonck Thiebaut
> >>>
> >>>
> >>> Please mind our privacy notice pursuant to Art. 13 GDPR. // Unsere Hinweise zum
> >>> Datenschutz gem. Art. 13 DSGVO finden Sie hier.
> >>
> >
> >
>
> Please mind our privacy notice<https://www.opensynergy.com/datenschutzerklaerung/privacy-notice-for-business-partners-pursuant-to-article-13-of-the-general-data-protection-regulation-gdpr/> pursuant to Art. 13 GDPR. // Unsere Hinweise zum Datenschutz gem. Art. 13 DSGVO finden Sie hier.<https://www.opensynergy.com/de/datenschutzerklaerung/datenschutzhinweise-fuer-geschaeftspartner-gem-art-13-dsgvo/>



-- 
Luiz Augusto von Dentz