Re: [PATCH v4 1/1] virtio_bt: Fix alignment in configuration struct

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



Hi Igor,

On Tue, Oct 25, 2022 at 1:17 PM Igor Skalkin
<igor.skalkin@xxxxxxxxxxxxxxx> wrote:
>
> Hi Luiz Augusto,
>
> On 10/24/22 22:54, Luiz Augusto von Dentz wrote:
> > Hi Igor,
> >
> > On Mon, Oct 24, 2022 at 6:41 AM Igor Skalkin
> > <Igor.Skalkin@xxxxxxxxxxxxxxx> wrote:
> >>
> >> The current version of the configuration structure has unaligned
> >> 16-bit fields, but according to the specification [1], access to
> >> the configuration space must be aligned.
> >>
> >> Add a second, aligned  version of the configuration structure
> >> and a new feature bit indicating that this version is being used.
> >>
> >> [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=db3482bc-5b84-4bde-bbb0-41d837955a7a&auth=53c7c7de28b92dfd96e93d9dd61a23e634d2fbec-d27a9d4c2c971f9ecc5d00d40d5cd9b45c4b5f63
> >>
> >> Signed-off-by: Igor Skalkin <Igor.Skalkin@xxxxxxxxxxxxxxx>
> >> ---
> >>  drivers/bluetooth/virtio_bt.c  | 16 +++++++++++++---
> >>  include/uapi/linux/virtio_bt.h |  8 ++++++++
> >>  2 files changed, 21 insertions(+), 3 deletions(-)
> >>
> >> diff --git a/drivers/bluetooth/virtio_bt.c b/drivers/bluetooth/virtio_bt.c
> >> index 67c21263f9e0..35f8041722c8 100644
> >> --- a/drivers/bluetooth/virtio_bt.c
> >> +++ b/drivers/bluetooth/virtio_bt.c
> >> @@ -306,7 +306,12 @@ static int virtbt_probe(struct virtio_device *vdev)
> >>         if (virtio_has_feature(vdev, VIRTIO_BT_F_VND_HCI)) {
> >>                 __u16 vendor;
> >>
> >> -               virtio_cread(vdev, struct virtio_bt_config, vendor, &vendor);
> >> +               if (virtio_has_feature(vdev, VIRTIO_BT_F_CONFIG_V2))
> >> +                       virtio_cread(vdev, struct virtio_bt_config_v2,
> >> +                                    vendor, &vendor);
> >> +               else
> >> +                       virtio_cread(vdev, struct virtio_bt_config,
> >> +                                    vendor, &vendor);
> >>
> >>                 switch (vendor) {
> >>                 case VIRTIO_BT_CONFIG_VENDOR_ZEPHYR:
> >> @@ -339,8 +344,12 @@ static int virtbt_probe(struct virtio_device *vdev)
> >>         if (virtio_has_feature(vdev, VIRTIO_BT_F_MSFT_EXT)) {
> >>                 __u16 msft_opcode;
> >>
> >> -               virtio_cread(vdev, struct virtio_bt_config,
> >> -                            msft_opcode, &msft_opcode);
> >> +               if (virtio_has_feature(vdev, VIRTIO_BT_F_CONFIG_V2))
> >> +                       virtio_cread(vdev, struct virtio_bt_config_v2,
> >> +                                    msft_opcode, &msft_opcode);
> >> +               else
> >> +                       virtio_cread(vdev, struct virtio_bt_config,
> >> +                                    msft_opcode, &msft_opcode);
> >>
> >>                 hci_set_msft_opcode(hdev, msft_opcode);
> >>         }
> >> @@ -387,6 +396,7 @@ static const unsigned int virtbt_features[] = {
> >>         VIRTIO_BT_F_VND_HCI,
> >>         VIRTIO_BT_F_MSFT_EXT,
> >>         VIRTIO_BT_F_AOSP_EXT,
> >> +       VIRTIO_BT_F_CONFIG_V2,
> >>  };
> >
> > So this introduces a new flag which must be checked when attempting to
> > config, right? But is this backward compatible? What happens if for
> > some reason the userspace doesn't use the new struct are we able to
> > detect that?
>
> Yes, it's backwards compatible.
>
> [q]Each virtio device offers all the features it understands. During
> device initialization, the driver reads this and tells the device the
> subset that it accepts. The only way to renegotiate is to reset the device.
> This allows for forwards and backwards compatibility: if the device is
> enhanced with a new feature bit, older drivers will not write that
> feature bit back to the device. Similarly, if a driver is enhanced with
> a feature that the device doesn’t support, it see the new feature is not
> offered.[/q]
>
> So, in our case:
>
> old device - new driver:
> The device does not offer VIRTIO_BT_F_CONFIG_V2 feature and uses the old
> configuration structure.
> The driver also uses the old configuration structure because
> VIRTIO_BT_F_CONFIG_V2 bit was not negotiated.
>
> new device - old driver:
> The device offers this bit, the driver reads it but cannot support it,
> so it does not write this bit back to the device during feature negotiation.
> The device verifies that this bit is not negotiated and continues to use
> the old configuration structure.
>
>
> I tested this patch, it
> a) works fine with a new device that supports VIRTIO_BT_F_CONFIG_V2.
> b) uses the old configuration structure when working with an old device.
>    Our device does not offer the VIRTIO_BT_F_VND_HCI feature bit, so the
> driver does not tries to read unaligned "vendor" and "msft_opcode"
> fields and everything is fine.
> But, if the VIRTIO_BT_F_VND_HCI feature is set for the device for test
> purposes, our middle layer asserts unaligned accesses to the
> configuration space.

Great, thanks for the explanation.

> P.S. But, as Michael S. Tsirkin rightly stated, [q]Will a spec patch be
> forthcoming?[/q], this patch requires a specification update.
> I could not find any virtio_bt specification, do you have one?
> That would be great. Otherwise, would you mind if I try to write some
> initial draft?

Yep, I don't think we have one so feel free to start one, also while
at it we could perhaps attempt to write a tester for it so we can test
it using our CI, assuming virtio works with virtual devices created by
vhci driver.

> >>  static struct virtio_driver virtbt_driver = {
> >> diff --git a/include/uapi/linux/virtio_bt.h b/include/uapi/linux/virtio_bt.h
> >> index a7bd48daa9a9..af798f4c9680 100644
> >> --- a/include/uapi/linux/virtio_bt.h
> >> +++ b/include/uapi/linux/virtio_bt.h
> >> @@ -9,6 +9,7 @@
> >>  #define VIRTIO_BT_F_VND_HCI    0       /* Indicates vendor command support */
> >>  #define VIRTIO_BT_F_MSFT_EXT   1       /* Indicates MSFT vendor support */
> >>  #define VIRTIO_BT_F_AOSP_EXT   2       /* Indicates AOSP vendor support */
> >> +#define VIRTIO_BT_F_CONFIG_V2  3       /* Use second version configuration */
> >>
> >>  enum virtio_bt_config_type {
> >>         VIRTIO_BT_CONFIG_TYPE_PRIMARY   = 0,
> >> @@ -28,4 +29,11 @@ struct virtio_bt_config {
> >>         __u16 msft_opcode;
> >>  } __attribute__((packed));
> >>
> >> +struct virtio_bt_config_v2 {
> >> +       __u8  type;
> >> +       __u8  alignment;
> >> +       __u16 vendor;
> >> +       __u16 msft_opcode;
> >> +};
> >> +
> >>  #endif /* _UAPI_LINUX_VIRTIO_BT_H */
> >> --
> >> 2.37.2
>


-- 
Luiz Augusto von Dentz




[Index of Archives]     [Bluez Devel]     [Linux Wireless Networking]     [Linux Wireless Personal Area Networking]     [Linux ATH6KL]     [Linux USB Devel]     [Linux Media Drivers]     [Linux Audio Users]     [Linux Kernel]     [Linux SCSI]     [Big List of Linux Books]

  Powered by Linux