Hi Andrea,
Thank you for your very detailed reply! I'm going to send a V2 which
should address all your comments.
On Tue, Dec 14, 2021 at 12:28 PM Andrea Parri <parri.andrea@xxxxxxxxx> wrote:
>
> On Tue, Dec 14, 2021 at 03:06:58AM +0100, Andrea Parri wrote:
[...]
> > In fact, IIUC, similar considerations would apply to hv_fb and hv_drm *if
> > it were not for the fact that those drivers seem to have bogus values for
> > max_pkt_size:
> >
> > 6) drivers/video/fbdev/hyperv_fb.c
> > (bufferlen = MAX_VMBUS_PKT_SIZE, max_pkt_size=VMBUS_DEFAULT_MAX_PKT_SIZE)
> >
> > 7) drivers/gpu/drm/hyperv/hyperv_drm_proto.c
> > (bufferlen = MAX_VMBUS_PKT_SIZE, max_pkt_size=VMBUS_DEFAULT_MAX_PKT_SIZE)
> >
Nice catch! The reason why I tried to apply a delta in vmbus code is I
found that VMBUS_DEFAULT_MAX_PKT_SIZE helps hide where should we set
max_pkt_size (so that drivers like hv_balloon is not changed), but I
failed to realize there are more.
> > So, IIUC, some separate patch is needed in order to "adjust" those values
> > (say, by appropriately setting max_pkt_size in synthvid_connect_vsp() and
> > in hyperv_connect_vsp()), but I digress.
> >
> > Other comments on your patch:
> >
> > a) You mentioned the problem that "pkt_offset may not match the packet
> > descriptor size". I think this is a real problem. To address this
> > problem, we can *presumably consider HV_HYP_PAGE_SIZE to be a valid
> > upper bound for pkt_offset (and sizeof(struct vmpacket_descriptor))
> > *and increase the value of max_pkt_size by HV_HYP_PAGE_SIZE (rather
> > than sizeof(struct vmpacket_descriptor)), like in:
> >
IIUC, pkt_offset is used for being forward-compatible with future
Hyper-V which may expand vmpacket_descriptor. If so, isn't a whole
page a little bit too much?
Anyway, I'm going to introduce a new #define for that, presumably
VMBUS_MAX_PKT_DESCR_SIZE, set to HY_HYP_PAGE_SIZE for now.
> > @@ -256,6 +256,7 @@ int hv_ringbuffer_init(struct hv_ring_buffer_info *ring_info,
> >
> > /* Initialize buffer that holds copies of incoming packets */
> > if (max_pkt_size) {
> > + max_pkt_size += HV_HYP_PAGE_SIZE;
> > ring_info->pkt_buffer = kzalloc(max_pkt_size, GFP_KERNEL);
> > if (!ring_info->pkt_buffer)
> > return -ENOMEM;
> >
> > b) We may then want to "enforce"/check that that bound on pkt_offset,
> >
> > pkt_offset <= HV_HYP_PAGE_SIZE,
> >
> > is met by adding a corresponding check to the (previously discussed)
> > validation of pkt_offset included in hv_pkt_iter_first(), say:
> >
> > @@ -498,7 +498,8 @@ struct vmpacket_descriptor *hv_pkt_iter_first(struct vmbus_channel *channel)
> > * If pkt_offset is invalid, arbitrarily set it to
> > * the size of vmpacket_descriptor
> > */
> > - if (pkt_offset < sizeof(struct vmpacket_descriptor) || pkt_offset > pkt_len)
> > + if (pkt_offset < sizeof(struct vmpacket_descriptor) || pkt_offset > pkt_len ||
> > + pkt_offset > HV_HYP_PAGE_SIZE)
> > pkt_offset = sizeof(struct vmpacket_descriptor);
> >
> > /* Copy the Hyper-V packet out of the ring buffer */
> >
> > While there (since I have noticed that such validation as well the
> > validation on pkt_len in hv_pkt_iter_first() tend to be the object
> > of a somehow recurring discussion ;/), I wouldn't mind to add some
> > "explicit" debug, pr_err_ratelimited() say, there.
Good idea!
> >
> > c) Last but not least, I'd recommend to pair the above changes or any
> > other change with some inline explanation/comments; these comments
> > could be snippets from an (updated) patch description for example.
Sure, thanks for the detailed guide here!
> >
> > Andrea
>
> One more thought. The additional HV_HYP_PAGE_SIZE seems unnecessary for
> drivers such as hv_netvsc and hv_storvsc, which explicitly account for
> pkt_offset in their setting of max_pkt_size, as well as for drivers such
> as hv_pci, which uses vmbus_recvpacket_raw().
>
> This suggests an alternative approach: do not increase max_pkt_size in
> the generic vmbus code, instead set/adjust max_pkt_size (only) for the
> drivers, in (1-7) above, which require the additional HV_HYP_PAGE_SIZE
> /pkt_offset. While putting on the driver(s) some additional "burden",
> this approach has the advantage of saving some memory (and keeping the
> generic code relatively simpler).
>
> Thoughts?
Provided that there are indeed drivers (hv_storvsc and hv_netvsc)
which explicitly account for vmpacket_descriptor header, changing
max_pkt_size for individual drivers makes more sense.
However in this case I'm not sure about our reasoning of 'pkt_offset'
above. In drivers/scsi/storvsc_drv.c:
#define STORVSC_MAX_PKT_SIZE (sizeof(struct vmpacket_descriptor) +\
sizeof(struct vstor_packet))
Should I also change this 'sizeof(struct vmpacket_descriptor)' to
VMBUS_MAX_PKT_DESCR_SIZE? Otherwise this would not match the check in
hv_pkt_iter_first.
>
> Andrea
Regards,
Yanming
