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
