Laszlo go ahead for the v5 to make the change you would like to see. I have really appreciated the amount of time you have spent on reviewing the patchset. Making your own changes would save you some of your time.
Jordan, I think your coding style comment has been lost in the review process. I have tried to address all the comment but I cannot recall these specific comments. Let me know your concern and I will send a patch once the patchset have been merged. > -----Original Message----- > From: Jordan Justen [mailto:jljus...@gmail.com] > Sent: 22 November 2013 00:39 > To: Laszlo Ersek > Cc: edk2-devel@lists.sourceforge.net; Olivier Martin; Drew Jones; > msal...@redhat.com; Paolo Bonzini; Kevin Wolf > Subject: Re: [edk2] [PATCH v4 00/11] OvmfPkg: Introduce and use the new > VIRTIO_DEVICE_PROTOCOL protocol > > On Thu, Nov 21, 2013 at 9:17 AM, Laszlo Ersek <ler...@redhat.com> > wrote: > > Jordan, > > > > On 11/10/13 03:15, Laszlo Ersek wrote: > >> On 11/08/13 19:48, Jordan Justen wrote: > >>> On Wed, Oct 30, 2013 at 5:51 PM, Laszlo Ersek <ler...@redhat.com> > wrote: > >>>> On 10/31/13 00:26, Jordan Justen wrote: > >>>> > >>>>> This protocol interface doesn't seem to follow the conventions of > the > >>>>> other io protocols. In addition, the alignment issue seems > unresolved > >>>>> to me. > >>>>> > >>>>> I just don't want any code outside of EDK II to pick this > interface > >>>>> up, and become dependent on it. > >>>>> > >>>>> Changing the width to be enum based, and omitting 64-bit accesses > >>>>> seems to resolve my immediate concerns. > >>>> > >>>> I think that changing the width to be enum based would eliminate a > >>>> safety feature of the (device specific) VIRTIO_CFG_READ() macros - > - we > >>>> could no longer check if the size of the pointer target, and the > size of > >>>> the device-specific field, are equal. The driver writer would have > to > >>>> specify the field width (the enum) each time. I think that's more > >>>> brittle than what we have now. > >>> > >>> It seems like a matter of preference. I'm not a huge fan of the > enum > >>> width in PciIo/CpuIo, but I'd rather just do things the 'UEFI way' > for > >>> consistency. > >> > >> No objection on my part. > >> > >>>>> I would also accept adding a disclaimer comment in the protocol > file, such as: > >>>>> "This protocol is a work in progress, and should not be used > outside > >>>>> of the EDK II tree." > >>>>> Unfortunately, I think it would be all to easy to miss such a > disclaimer. > >>>> > >>>> This is positively how I've approached the new protocol. In my > eyes it's > >>>> nothing to standardize or advertise or whatever. It's only a > protocol > >>>> because that's how we implement virtual functions (a structure of > >>>> function pointers) in UEFI, while remaining compatible with the > driver > >>>> model (automatic discovery etc). That's all. > >>>> > >>>> The protocol serves our direct needs. We have two implementations, > and > >>>> three clients. I don't believe in early generalization. > >>> > >>> It seems like there is a fair amount of interest in virtio. Can we > at > >>> least make it clear that this protocol interface is only half- > baked, > >>> and therefore should not be widely relied upon? > >> > >> Yes, we should add such a disclaimer. I guess people trying to use > the > >> protocol would look up the struct definition with the members, so > that's > >> probably a good place. (This is what you suggested too, in my > >> understanding.) > > For all Olivier's OvmfPkg patches, except the protocol definition: > Reviewed-by: Jordan Justen <jordan.l.jus...@intel.com> > (I did have comment code style feedback that was not addressed, but, > oh well. Nits...) > > For the protocol definition, I think the current best idea to move > forward is to just add the disclaimer as a comment. I'd like to see it > at the top of the protocol file. I also think your suggestion of > putting it near the structure is good. So how about in both spots? :) > > There is probably no need to change the protocol definition until we > decide to put serious thought into it. For now, I don't think that has > happened, so the disclaimer is probably sufficient. > > -Jordan > > > If I fix these two things that you'd like, and also fix up the > problems > > I myself have pointed out, and submit a v5, will you apply it? > > > > I'm asking for several reasons: > > - I've put a great deal of effort into the review. I don't want to > have > > it wasted. > > > > - The series is great. It needs to go in. > > > > - I'd like to update VirtioBlkDxe to export revision 2 and maybe > > revision 3 blockio media stuff. > > > > Please see VIRTIO_BLK_F_TOPOLOGY and "struct virtio_blk_topology" in > > <https://tools.oasis-open.org/version-control/svn/virtio/virtio-v1.0- > wd01-part1-specification.txt>. > > > > > > Such a change would conflict with this series. > > > > Thanks, > > Laszlo ------------------------------------------------------------------------------ Rapidly troubleshoot problems before they affect your business. Most IT organizations don't have a clear picture of how application performance affects their revenue. With AppDynamics, you get 100% visibility into your Java,.NET, & PHP application. Start your 15-day FREE TRIAL of AppDynamics Pro! http://pubads.g.doubleclick.net/gampad/clk?id=84349351&iu=/4140/ostg.clktrk _______________________________________________ edk2-devel mailing list edk2-devel@lists.sourceforge.net https://lists.sourceforge.net/lists/listinfo/edk2-devel
