On 08/21/2012 09:35 AM, Alexander Graf wrote: > > On 21.08.2012, at 04:32, Rusty Russell <rusty.russell@xxxxxxxxxx> wrote: > >> On Tue, 21 Aug 2012 00:34:59 +0200, Alexander Graf <agraf@xxxxxxx> wrote: >>> On 21.08.2012, at 00:28, Peter Maydell wrote: >>> >>>> On 20 August 2012 23:18, Alexander Graf <agraf@xxxxxxx> wrote: >>>>> My question still holds. Why not use ONE_REG for this? What advantage >>>>> does GET/SET_MSRS give you? >>>> It lets you set more than one register at once, and it follows >>>> the pattern x86 already has (with a GET, SET, and a LIST_INDEX >>>> for finding out what the kernel actually supports). >>> Maybe we should sit down and generalize those features and make them >>> available to everyone rather than shoehorning ARM into an x86 only >>> ioctl then? >> See, you tried to create a generic ioctl, and failed. So you have a PPC >> specific one. > The whole ONE_REG API is pretty new. Even for PPC we don't use it as often as I would like. > >> And you're smarter than us. If we try to create a generic ioctl, we too >> will fail, and end up with an ARM-specific one. > I don't get your point. You're trying to solve a very simple problem that everyone has: Synchronize register state. And in fact, your world is a lot closer to PPC than x86, since the architecture is changing in more extreme ways too. > > Let me tell you the background on why I went with the concept as it is. > > At first, things were pretty dumbly set up. Kernel space had some hardcoded values. User space also did. Over time, every time I would find out that I actually need to access a register from user space, I would add it to the sregs struct. We even had some awesome code in there to only partially update smaller bits of state instead of the whole struct. > > Then came the day when I added a field to the struct beyond its initial padding. And boom we had a different ioctl. So I sat down and tried to come up with an API that would allow us not to shoot ourselves in the foot for everything we do. That's what ONE_REG is. It's a simple reservation of number space. We could easily add new functionality on top: > > - enumerare which regs are supported > - change multiple regs with one ioctl > - have a (pointer, reg) list deposited in kernel space that gets updates without new ioctl whenever state changes on user space exit > > There are plenty things we could do. And they would always apply to all archs. Because the underlying problem is always the same... > >> Now, if Anthony and Avi lay down the law and say everyone is moving to a >> new system, we'll move now. For real: I want to see x86 patches. > So because x86 has a stable API today that works for them and that needs to be supported anyways we don't improve the non-x86 code? What an awesome argument. I should bring that one up next time I don't want to think about something. > >> Otherwise, we'll concentrate on stuff we know, and transition when >> everyone else does. >> >> And since that transition is likely to be painful and of no real >> benefit, it might never happen. > That's exactly my point. Today you have the chance and could go with something generic, extensible. > > In fact, you basically reimplemented ONE_REG on top of the x86 MSR ioctls. You sliced up the available number space, just without leaving space for non-arm numbers. And you shoved all of that into ioctls that are named after something very x86 specific: MSRs. > > So either try to work with everyone on a solution for everyone, rename the ioctls to something more generic, or invent new ones for you. But don't create another arm-one-reg api under the MSR code name. Also if you're wondering why the MSR stuff is not marked as x86 specific, that's because initially there were 2 targets in KVM using it: x86 and IA64. Both implement MSRs, both call them MSRs. So it made sense to create a generic framework out of it. It was not meant to be used for architectures that call it differently ;). Alex _______________________________________________ kvmarm mailing list kvmarm@xxxxxxxxxxxxxxxxxxxxx https://lists.cs.columbia.edu/cucslists/listinfo/kvmarm
