Re: [PATCH 3/5] tools/vhost-user-i2c: Add backend driver

[Alex Bennée]

Viresh Kumar <viresh.kumar@linaro.org> writes:

> On 25-03-21, 13:09, Jie Deng wrote:
>> 
>> On 2021/3/24 15:33, Viresh Kumar wrote:
>> > +
>> > +/* Definitions from virtio-i2c specifications */
>> > +#define VHOST_USER_I2C_MAX_QUEUES       1
>> > +
>> > +/* Status */
>> > +#define VIRTIO_I2C_MSG_OK               0
>> > +#define VIRTIO_I2C_MSG_ERR              1
>> > +
>> > +/* The bit 0 of the @virtio_i2c_out_hdr.@flags, used to group the 
>> > requests */
>> > +#define VIRTIO_I2C_FLAGS_FAIL_NEXT      0x00000001
>> > +
>> > +/**
>> > + * struct virtio_i2c_out_hdr - the virtio I2C message OUT header
>> > + * @addr: the controlled device's address
>> > + * @padding: used to pad to full dword
>> > + * @flags: used for feature extensibility
>> > + */
>> > +struct virtio_i2c_out_hdr {
>> > +    uint16_t addr;
>> > +    uint16_t padding;
>> > +    uint32_t flags;
>> > +} __attribute__((packed));
>> 
>> 
>> __le16,  __le32 ?
>
> Maybe, but I didn't do them because of this:
>
> docs/devel/style.rst:
>
> "Don't use Linux kernel internal types like u32, __u32 or __le32."
>  
>> > +
>> > +/**
>> > + * struct virtio_i2c_in_hdr - the virtio I2C message IN header
>> > + * @status: the processing result from the backend
>> > + */
>> > +struct virtio_i2c_in_hdr {
>> > +    uint8_t status;
>> > +} __attribute__((packed));
>> > +
>> 
>> I understand these definitions can be removed once the frontend driver is
>> merged by the Linux ?
>
> Yes, we would be required to somehow include the uapi header that
> kernel is adding and then this won't be required.

What I often do is include a temporary patch in my series that includes
the updated uapi headers from my Linux branch and mark it as not for
merge until the uapi headers make it into a released tree.

>  
>> > +/* vhost-user-i2c definitions */
>> > +
>> > +#ifndef container_of
>> > +#define container_of(ptr, type, member) ({                      \
>> > +        const typeof(((type *) 0)->member) *__mptr = (ptr);     \
>> > +        (type *) ((char *) __mptr - offsetof(type, member));})
>> > +#endif
>> 
>> 
>> This seems to be a general interface.  I see there is a definition in
>> qemu/compiler.h.
>> 
>> Can we reuse it ?
>
> Damn. My bad (maybe not). I picked this part from the RPMB patchset
> that Alex sent and didn't bother looking for it.
>
> Though on the other hand, we are looking to make this file independent
> of qemu so it can be used by other hypervisors without any (or much)
> modifications, and maybe so it was done so.
>
> Alex ?


-- 
Alex Bennée